On Mon, Aug 4, 2025 at 11:18 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Sun, Aug 3, 2025 at 3:03 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > After testing, the patch LGTM. I noticed two very small possible nits:
>
> Thanks for the review!
>
>
> > 1) Comment wording
> >
> > The loop now calls isspace((unsigned char)*ptr), so a token ends at
> > any whitespace, not just at ASCII space (0x20). Could we revise the
> > comment—from
> > “strings of non-space characters bounded by space characters”
> > to something like
> > “strings of non-space characters bounded by whitespace”
> > —to match the behavior?
>
> I agree with the change. But the phrase "strings of non-space characters
> bounded by whitespace" is a bit redundant, and "strings of non-whitespace
> characters" is sufficient, isn't it? So I used that wording in the updated
> patch I've attached.
>
>
> > 2) Variable name
> >
> > const char *keyword = filter_get_token(&str, &size);
> > keyword = filter_get_token(&str, &size);
> >
> > After the patch, filter_get_token() no longer returns a keyword
> > (letters-only identifier); it now returns any non-whitespace token.
> > Renaming the variable from keyword to token (or similar) might make
> > the intent clearer..
>
> This also got me thinking, if we simply define keywords as strings of
> non-whitespace characters, maybe we don't need to change the term "keyword"
> to "token" at all. I've updated the patch with that in mind. Thoughts?
>
+1, this looks more elegant to me.
Best,
Xuneng