On Mon, Aug 4, 2025 at 11:18 PM Fujii Masao <masao.fu...@gmail.com> wrote: > > On Sun, Aug 3, 2025 at 3:03 PM Xuneng Zhou <xunengz...@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
