| Message ID | 20220817200335.911-2-kabel@kernel.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mvebu a3720 comphy: Fix serdes transmit amplitude | expand |
On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <kabel@kernel.org> wrote: > > Add str_has_proper_prefix(), similar to str_has_prefix(), but requires > that the prefix is proper: the string itself must be longer than the > prefix. > > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > Andy, Kees, could you ack this if it is ok? Seems to me there are too many strlen():s. One is hidden in strncmp(). Besides not the good naming (what 'proper' means), the entire function is not needed. You may simply call str_has_prefix() && p[len] != '\0'; Correct?
On Thu, 18 Aug 2022 22:10:58 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <kabel@kernel.org> wrote: > > > > Add str_has_proper_prefix(), similar to str_has_prefix(), but requires > > that the prefix is proper: the string itself must be longer than the > > prefix. > > > > Signed-off-by: Marek Behún <kabel@kernel.org> > > --- > > Andy, Kees, could you ack this if it is ok? > > Seems to me there are too many strlen():s. One is hidden in strncmp(). I thought this was ok cause gcc has optimizations for this in https://github.com/gcc-mirror/gcc/blob/master/gcc/tree-ssa-strlen.cc But now I see that kernel does not declare these functions as inline that call __builtin_strlen()... so probably the optimizations are not used. > Besides not the good naming (what 'proper' means), The naming comes from similar naming in math: proper subset is as subset that is not equal to the superset. See https://en.wikipedia.org/wiki/Substring : "A proper prefix of a string is not equal to the string itself" > the entire function is not needed. You may simply call > > str_has_prefix() && p[len] != '\0'; > > Correct? Do you mean that I should implement this function to simply return str_has_prefix() && p[len] != '\0' or that this function should not exist at all and I should do that in the code where I would have used the function? Thanks. Marek
On Thu, Aug 18, 2022 at 10:48 PM Marek Behún <kabel@kernel.org> wrote: > On Thu, 18 Aug 2022 22:10:58 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <kabel@kernel.org> wrote: ... > > Besides not the good naming (what 'proper' means), > > The naming comes from similar naming in math: proper subset is as > subset that is not equal to the superset. See > https://en.wikipedia.org/wiki/Substring : > "A proper prefix of a string is not equal to the string itself" It's nice to learn something, but I still think that name has too broad meaning(s) that may easily confuse the developers. > > > the entire function is not needed. You may simply call > > > > str_has_prefix() && p[len] != '\0'; > > > > Correct? > > Do you mean that I should implement this function to simply return > str_has_prefix() && p[len] != '\0' > or that this function should not exist at all and I should do that in > the code where I would have used the function? The latter since this seems do not have users, except a single newcomer,
On Thu, 18 Aug 2022 22:56:14 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Aug 18, 2022 at 10:48 PM Marek Behún <kabel@kernel.org> wrote: > > On Thu, 18 Aug 2022 22:10:58 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <kabel@kernel.org> wrote: > > ... > > > > Besides not the good naming (what 'proper' means), > > > > The naming comes from similar naming in math: proper subset is as > > subset that is not equal to the superset. See > > https://en.wikipedia.org/wiki/Substring : > > "A proper prefix of a string is not equal to the string itself" > > It's nice to learn something, but I still think that name has too > broad meaning(s) that may easily confuse the developers. > > > > > > the entire function is not needed. You may simply call > > > > > > str_has_prefix() && p[len] != '\0'; > > > > > > Correct? > > > > Do you mean that I should implement this function to simply return > > str_has_prefix() && p[len] != '\0' > > or that this function should not exist at all and I should do that in > > the code where I would have used the function? > > The latter since this seems do not have users, except a single newcomer, > Very well. Thanks for the review. Marek
On Thu, 18 Aug 2022 22:56:14 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Aug 18, 2022 at 10:48 PM Marek Behún <kabel@kernel.org> wrote: > > On Thu, 18 Aug 2022 22:10:58 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Wed, Aug 17, 2022 at 11:06 PM Marek Behún <kabel@kernel.org> wrote: > > ... > > > > Besides not the good naming (what 'proper' means), > > > > The naming comes from similar naming in math: proper subset is as > > subset that is not equal to the superset. See > > https://en.wikipedia.org/wiki/Substring : > > "A proper prefix of a string is not equal to the string itself" > > It's nice to learn something, but I still think that name has too > broad meaning(s) that may easily confuse the developers. > > > > > > the entire function is not needed. You may simply call > > > > > > str_has_prefix() && p[len] != '\0'; > > > > > > Correct? > > > > Do you mean that I should implement this function to simply return > > str_has_prefix() && p[len] != '\0' > > or that this function should not exist at all and I should do that in > > the code where I would have used the function? > > The latter since this seems do not have users, except a single newcomer, > Just out of curiosity I grepped for all usages of str_has_prefix() and there are some where it str_has_proper_prefix() could detect failure earlier. For example in kernel/trace/trace_events_user.c in function user_field_size(). Do you think I should propose converting those? There aren't that many uses, so probably not. Marek
diff --git a/include/linux/string.h b/include/linux/string.h index 61ec7e4f6311..375f51b9182c 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -316,4 +316,22 @@ static __always_inline size_t str_has_prefix(const char *str, const char *prefix return strncmp(str, prefix, len) == 0 ? len : 0; } +/** + * str_has_proper_prefix - Test if a string has a proper prefix + * @str: The string to test + * @prefix: The string to see if @str starts with + * + * This is like str_has_prefix(), but fails if the strings are equal. + * + * Returns: + * * strlen(@prefix) if @str starts with @prefix and they aren't equal + * * 0 otherwise + */ +static __always_inline size_t str_has_proper_prefix(const char *str, + const char *prefix) +{ + size_t len = strlen(prefix); + return strncmp(str, prefix, len) == 0 && len != strlen(str) ? len : 0; +} + #endif /* _LINUX_STRING_H_ */
Add str_has_proper_prefix(), similar to str_has_prefix(), but requires that the prefix is proper: the string itself must be longer than the prefix. Signed-off-by: Marek Behún <kabel@kernel.org> --- Andy, Kees, could you ack this if it is ok? --- include/linux/string.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)