Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 21 Dec 2018 10:51:29 -0800 | Subject | Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro |
| |
On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > I figured the best thing to do is to create a helper macro and place it > into include/linux/string.h. And go around and fix all the open coded > versions of it later. > > I plan on only applying this patch and updating the tracing hooks for > this merge window. And perhaps use it to fix some of the bugs that were > found.
I like the helper function concept, but as they say about CS: "There is only one hard problem in computer science: naming and off-by-one errors".
And this one has that problem. The name makes absolutely no sense.
Calling it "strncmp_prefix()" when there is no "n" there makes no sense.
So drop the "n" from the name.
And honestly, maybe it would be better to use a different naming pattern entirely, and avoid the crazy non-boolean "str*cmp()" model. That thing is useful for search comparisons (where "before/after" matters), but it's horrible for the actual "is this true or not", where the code ends up being
if (!strcmp_prefix(a, "prefix")) { // This is the "Yes, prefix matched" case, despite the "if !" syntax
which is just confusing.
So I'd really prefer more of a
#define has_prefix(str, prefix) ...
model that actually returns a boolean (true if it has a prefix, false if it doesn't), rather than use the "str*cmp" naming and return values.
(But I agree that *if* you use the "strcmp" naming, then you do need to hold to the traditional strcmp return value semantics).
Hmm?
Finally, I also suspect that your helper might be slightly fragile. Doing sizeof() seems broken. I could see somebody using some prefix define for arrays with constant sizes, and doing
#define PFX1 "cpp\0" #define PFX2 "c\0\0\0" #define PFX3 "h\0\0\0"
or similar. Also, is there a reason you use "&prefix" for the constant test? I don't see that pattern anywhere else. Plus it looks entirely wrong without the parenthesis (ie "&(prefix)" to make sure we group things right).
So a lot of small problems, but the naming one is big.
Linus
| |