Re: master 67ab357cdcc 7/7: Support treesit-thing-settings in search functions

Re: master 67ab357cdcc 7/7: Support treesit-thing-settings in search functions

[Po Lu]

Yuan Fu <casouri@gmail.com> writes:

> +/* Assq but doesn't signal.  */
> +static Lisp_Object
> +safe_assq (Lisp_Object key, Lisp_Object alist)
> +{
> +  Lisp_Object tail = alist;
> +  FOR_EACH_TAIL_SAFE (tail)
> +    if (CONSP (XCAR (tail)) && EQ (XCAR (XCAR (tail)), key))
> +      return XCAR (tail);
> +  return Qnil;
> +}

Please, write more descriptive doc strings.  I would write:

  /* Like `assq_no_quit', except it avoids chasing after circular
     lists.  */

Re: master 67ab357cdcc 7/7: Support treesit-thing-settings in search functions

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Yuan Fu <casouri@gmail.com>
> Date: Sat, 15 Apr 2023 10:43:21 +0800
> 
> Yuan Fu <casouri@gmail.com> writes:
> 
> > +/* Assq but doesn't signal.  */
> > +static Lisp_Object
> > +safe_assq (Lisp_Object key, Lisp_Object alist)
> > +{
> > +  Lisp_Object tail = alist;
> > +  FOR_EACH_TAIL_SAFE (tail)
> > +    if (CONSP (XCAR (tail)) && EQ (XCAR (XCAR (tail)), key))
> > +      return XCAR (tail);
> > +  return Qnil;
> > +}
> 
> Please, write more descriptive doc strings.  I would write:
> 
>   /* Like `assq_no_quit', except it avoids chasing after circular
>      lists.  */

It is more efficient to just go ahead and make minor changes like
that, than start discussions about them.

Thanks.

Re: master 67ab357cdcc 7/7: Support treesit-thing-settings in search functions

[Eli Zaretskii]

> Date: Sat, 15 Apr 2023 09:58:09 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org, casouri@gmail.com
> 
> > From: Po Lu <luangruo@yahoo.com>
> > Cc: Yuan Fu <casouri@gmail.com>
> > Date: Sat, 15 Apr 2023 10:43:21 +0800
> > 
> > Yuan Fu <casouri@gmail.com> writes:
> > 
> > > +/* Assq but doesn't signal.  */
> > > +static Lisp_Object
> > > +safe_assq (Lisp_Object key, Lisp_Object alist)
> > > +{
> > > +  Lisp_Object tail = alist;
> > > +  FOR_EACH_TAIL_SAFE (tail)
> > > +    if (CONSP (XCAR (tail)) && EQ (XCAR (XCAR (tail)), key))
> > > +      return XCAR (tail);
> > > +  return Qnil;
> > > +}
> > 
> > Please, write more descriptive doc strings.  I would write:
> > 
> >   /* Like `assq_no_quit', except it avoids chasing after circular
> >      lists.  */
> 
> It is more efficient to just go ahead and make minor changes like
> that, than start discussions about them.

Btw, Yuan: any reason you couldn't use the existing assq_no_quit here?

Re: master 67ab357cdcc 7/7: Support treesit-thing-settings in search functions

[Yuan Fu]

> On Apr 15, 2023, at 2:58 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Date: Sat, 15 Apr 2023 09:58:09 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: emacs-devel@gnu.org, casouri@gmail.com
>> 
>>> From: Po Lu <luangruo@yahoo.com>
>>> Cc: Yuan Fu <casouri@gmail.com>
>>> Date: Sat, 15 Apr 2023 10:43:21 +0800
>>> 
>>> Yuan Fu <casouri@gmail.com> writes:
>>> 
>>>> +/* Assq but doesn't signal.  */
>>>> +static Lisp_Object
>>>> +safe_assq (Lisp_Object key, Lisp_Object alist)
>>>> +{
>>>> +  Lisp_Object tail = alist;
>>>> +  FOR_EACH_TAIL_SAFE (tail)
>>>> +    if (CONSP (XCAR (tail)) && EQ (XCAR (XCAR (tail)), key))
>>>> +      return XCAR (tail);
>>>> +  return Qnil;
>>>> +}
>>> 
>>> Please, write more descriptive doc strings.  I would write:
>>> 
>>>  /* Like `assq_no_quit', except it avoids chasing after circular
>>>     lists.  */
>> 
>> It is more efficient to just go ahead and make minor changes like
>> that, than start discussions about them.

And rest assured that I do read git logs and I’ll take note of changes you made. Eli and many others have fixes mistakes I made in the code, I’m very grateful and try my best to learn from it.

> 
> Btw, Yuan: any reason you couldn't use the existing assq_no_quit here?

To be very honest I didn’t know its existence before. But safe_assq is indeed necessary, because assq_no_quit assumes the list is not circular and doesn’t try to detect one.

For this particular use-case I just don’t want assq to signal any error, but I do want it to detect errors. Circular error, malformed alist, or simply couldn’t find the key, all of these will be reported to the user as “couldn’t find the key” (as an error). This is easier to understand for the user and easier for me to implement (than using unwind-protect and let assq signal).

Yuan

Re: master 67ab357cdcc 7/7: Support treesit-thing-settings in search functions

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Sat, 15 Apr 2023 22:54:29 -0700
> Cc: Po Lu <luangruo@yahoo.com>,
>  emacs-devel@gnu.org
> 
> > Btw, Yuan: any reason you couldn't use the existing assq_no_quit here?
> 
> To be very honest I didn’t know its existence before. But safe_assq is indeed necessary, because assq_no_quit assumes the list is not circular and doesn’t try to detect one.
> 
> For this particular use-case I just don’t want assq to signal any error, but I do want it to detect errors. Circular error, malformed alist, or simply couldn’t find the key, all of these will be reported to the user as “couldn’t find the key” (as an error). This is easier to understand for the user and easier for me to implement (than using unwind-protect and let assq signal).

OK, but please move the safe_assq function (under the name
assq_no_signal, I suggest) to where we have assq_no_quit, and please
explain the difference between them in the commentary.

Thanks.

Re: master 67ab357cdcc 7/7: Support treesit-thing-settings in search functions

[Yuan Fu]

> On Apr 15, 2023, at 11:37 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Sat, 15 Apr 2023 22:54:29 -0700
>> Cc: Po Lu <luangruo@yahoo.com>,
>> emacs-devel@gnu.org
>> 
>>> Btw, Yuan: any reason you couldn't use the existing assq_no_quit here?
>> 
>> To be very honest I didn’t know its existence before. But safe_assq is indeed necessary, because assq_no_quit assumes the list is not circular and doesn’t try to detect one.
>> 
>> For this particular use-case I just don’t want assq to signal any error, but I do want it to detect errors. Circular error, malformed alist, or simply couldn’t find the key, all of these will be reported to the user as “couldn’t find the key” (as an error). This is easier to understand for the user and easier for me to implement (than using unwind-protect and let assq signal).
> 
> OK, but please move the safe_assq function (under the name
> assq_no_signal, I suggest) to where we have assq_no_quit, and please
> explain the difference between them in the commentary.
> 
> Thanks.

For me it’s just a quick local helper function. Is it useful enough to be moved to fns.c?

Yuan

Re: master 67ab357cdcc 7/7: Support treesit-thing-settings in search functions

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Sat, 15 Apr 2023 23:42:01 -0700
> Cc: Po Lu <luangruo@yahoo.com>,
>  emacs-devel@gnu.org
> 
> 
> 
> > On Apr 15, 2023, at 11:37 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> >> From: Yuan Fu <casouri@gmail.com>
> >> Date: Sat, 15 Apr 2023 22:54:29 -0700
> >> Cc: Po Lu <luangruo@yahoo.com>,
> >> emacs-devel@gnu.org
> >> 
> >>> Btw, Yuan: any reason you couldn't use the existing assq_no_quit here?
> >> 
> >> To be very honest I didn’t know its existence before. But safe_assq is indeed necessary, because assq_no_quit assumes the list is not circular and doesn’t try to detect one.
> >> 
> >> For this particular use-case I just don’t want assq to signal any error, but I do want it to detect errors. Circular error, malformed alist, or simply couldn’t find the key, all of these will be reported to the user as “couldn’t find the key” (as an error). This is easier to understand for the user and easier for me to implement (than using unwind-protect and let assq signal).
> > 
> > OK, but please move the safe_assq function (under the name
> > assq_no_signal, I suggest) to where we have assq_no_quit, and please
> > explain the difference between them in the commentary.
> > 
> > Thanks.
> 
> For me it’s just a quick local helper function. Is it useful enough to be moved to fns.c?

Yes.  If it's useful in treesit.c, chances are it will be useful in
other places.

Alternatively, if you could use assq_no_quit, and test the additional
conditions in your own code (I don't know if this is possible), that
would also be good.

Re: master 67ab357cdcc 7/7: Support treesit-thing-settings in search functions

[Yuan Fu]

> On Apr 16, 2023, at 1:19 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Sat, 15 Apr 2023 23:42:01 -0700
>> Cc: Po Lu <luangruo@yahoo.com>,
>> emacs-devel@gnu.org
>> 
>> 
>> 
>>> On Apr 15, 2023, at 11:37 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> 
>>>> From: Yuan Fu <casouri@gmail.com>
>>>> Date: Sat, 15 Apr 2023 22:54:29 -0700
>>>> Cc: Po Lu <luangruo@yahoo.com>,
>>>> emacs-devel@gnu.org
>>>> 
>>>>> Btw, Yuan: any reason you couldn't use the existing assq_no_quit here?
>>>> 
>>>> To be very honest I didn’t know its existence before. But safe_assq is indeed necessary, because assq_no_quit assumes the list is not circular and doesn’t try to detect one.
>>>> 
>>>> For this particular use-case I just don’t want assq to signal any error, but I do want it to detect errors. Circular error, malformed alist, or simply couldn’t find the key, all of these will be reported to the user as “couldn’t find the key” (as an error). This is easier to understand for the user and easier for me to implement (than using unwind-protect and let assq signal).
>>> 
>>> OK, but please move the safe_assq function (under the name
>>> assq_no_signal, I suggest) to where we have assq_no_quit, and please
>>> explain the difference between them in the commentary.
>>> 
>>> Thanks.
>> 
>> For me it’s just a quick local helper function. Is it useful enough to be moved to fns.c?
> 
> Yes.  If it's useful in treesit.c, chances are it will be useful in
> other places.

Thanks. I did that.

> 
> Alternatively, if you could use assq_no_quit, and test the additional
> conditions in your own code (I don't know if this is possible), that
> would also be good.

I won’t bother you with details but assq_no_signal is necessary ;-)

Yuan