* [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings @ 2017-09-29 15:37 Stephen Hemminger 2017-10-02 10:01 ` Luca Boccassi 2017-10-02 11:53 ` Adrien Mazarguil 0 siblings, 2 replies; 11+ messages in thread From: Stephen Hemminger @ 2017-09-29 15:37 UTC (permalink / raw) To: dev; +Cc: Stephen Hemminger, Stephen Hemminger The Linux kernel style policy about strings is that strings should be always put on one line. This makes sense since a typical use case is for a user to type the error message into a search engine or grep, and it won't be found if split across lines. This patch just re-enables that check. Yes, lots of DPDK code now splits strings, that doesn't make it right. Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh index a56c41a301c0..3e6081dd673e 100755 --- a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 +44,6 @@ options="$options --show-types" options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ -SPLIT_STRING,LONG_LINE_STRING,\ LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ NEW_TYPEDEFS,COMPARISON_TO_NULL" -- 2.11.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2017-09-29 15:37 [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings Stephen Hemminger @ 2017-10-02 10:01 ` Luca Boccassi 2017-10-02 11:53 ` Adrien Mazarguil 1 sibling, 0 replies; 11+ messages in thread From: Luca Boccassi @ 2017-10-02 10:01 UTC (permalink / raw) To: Stephen Hemminger, dev On Fri, 2017-09-29 at 08:37 -0700, Stephen Hemminger wrote: > The Linux kernel style policy about strings is that strings should > be always put on one line. This makes sense since a typical use > case is for a user to type the error message into a search engine > or grep, and it won't be found if split across lines. > This patch just re-enables that check. > > Yes, lots of DPDK code now splits strings, that doesn't > make it right. > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > --- > devtools/checkpatches.sh | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > index a56c41a301c0..3e6081dd673e 100755 > --- a/devtools/checkpatches.sh > +++ b/devtools/checkpatches.sh > @@ -44,7 +44,6 @@ options="$options --show-types" > options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ > VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ > PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ > -SPLIT_STRING,LONG_LINE_STRING,\ > LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ > NEW_TYPEDEFS,COMPARISON_TO_NULL" > Acked-by: Luca Boccassi <bluca@debian.org> +1 - being able to reliably Google/grep across such a large code base is extremely useful -- Kind regards, Luca Boccassi ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2017-09-29 15:37 [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings Stephen Hemminger 2017-10-02 10:01 ` Luca Boccassi @ 2017-10-02 11:53 ` Adrien Mazarguil 2017-10-02 13:46 ` Bruce Richardson 1 sibling, 1 reply; 11+ messages in thread From: Adrien Mazarguil @ 2017-10-02 11:53 UTC (permalink / raw) To: Stephen Hemminger; +Cc: dev Hi Stephen, On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: > The Linux kernel style policy about strings is that strings should > be always put on one line. This makes sense since a typical use > case is for a user to type the error message into a search engine > or grep, and it won't be found if split across lines. > This patch just re-enables that check. > > Yes, lots of DPDK code now splits strings, that doesn't > make it right. > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > --- > devtools/checkpatches.sh | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > index a56c41a301c0..3e6081dd673e 100755 > --- a/devtools/checkpatches.sh > +++ b/devtools/checkpatches.sh > @@ -44,7 +44,6 @@ options="$options --show-types" > options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ > VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ > PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ > -SPLIT_STRING,LONG_LINE_STRING,\ > LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ > NEW_TYPEDEFS,COMPARISON_TO_NULL" I'm not sure, given that the main reason for splitting strings in the first place is to avoid LONG_LINE_STRING warnings, I think we must choose between the two options. If split strings are not allowed, then long lines must be. Since checkpatches.sh is used by various automated scripts to complain loudly about problems in submissions, the above change prevents maintainers from writing long string at all (can't split and can't go past 80 columns). As a result, they will be tempted to cripple their code with nasty workarounds to shut up checkpatches.sh, we don't want that to happen. Also I think the reasons stated by original commit cf75514c8e2e are still relevant. My vote would be to keep things as is. -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2017-10-02 11:53 ` Adrien Mazarguil @ 2017-10-02 13:46 ` Bruce Richardson 2017-10-02 16:21 ` Adrien Mazarguil 0 siblings, 1 reply; 11+ messages in thread From: Bruce Richardson @ 2017-10-02 13:46 UTC (permalink / raw) To: Adrien Mazarguil; +Cc: Stephen Hemminger, dev On Mon, Oct 02, 2017 at 01:53:17PM +0200, Adrien Mazarguil wrote: > Hi Stephen, > > On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: > > The Linux kernel style policy about strings is that strings should > > be always put on one line. This makes sense since a typical use case > > is for a user to type the error message into a search engine or > > grep, and it won't be found if split across lines. This patch just > > re-enables that check. > > > > Yes, lots of DPDK code now splits strings, that doesn't make it > > right. > > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- > > devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) > > > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > > index a56c41a301c0..3e6081dd673e 100755 --- > > a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 > > +44,6 @@ options="$options --show-types" options="$options > > --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ > > VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ > > PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ > > -SPLIT_STRING,LONG_LINE_STRING,\ > > LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ > > NEW_TYPEDEFS,COMPARISON_TO_NULL" > > I'm not sure, given that the main reason for splitting strings in the > first place is to avoid LONG_LINE_STRING warnings, I think we must > choose between the two options. If split strings are not allowed, then > long lines must be. > > Since checkpatches.sh is used by various automated scripts to complain > loudly about problems in submissions, the above change prevents > maintainers from writing long string at all (can't split and can't go > past 80 columns). > > As a result, they will be tempted to cripple their code with nasty > workarounds to shut up checkpatches.sh, we don't want that to happen. > > Also I think the reasons stated by original commit cf75514c8e2e are > still relevant. My vote would be to keep things as is. > In my experience, checkpatch is smart enough to recognise when a long line overflows the 80 character limit because of a single long string, so the two options are not mutually exclusive. In other words, long lines are not allowed except in the case where shortening the line involves splitting a string. There may be a small amount of work in getting checkpatch happy, i.e. by putting the string on a line on it's own, but we can indeed have our cake and eat it too in this case. /Bruce ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2017-10-02 13:46 ` Bruce Richardson @ 2017-10-02 16:21 ` Adrien Mazarguil 2017-10-03 10:38 ` Bruce Richardson 0 siblings, 1 reply; 11+ messages in thread From: Adrien Mazarguil @ 2017-10-02 16:21 UTC (permalink / raw) To: Bruce Richardson; +Cc: Stephen Hemminger, dev On Mon, Oct 02, 2017 at 02:46:24PM +0100, Bruce Richardson wrote: > On Mon, Oct 02, 2017 at 01:53:17PM +0200, Adrien Mazarguil wrote: > > Hi Stephen, > > > > On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: > > > The Linux kernel style policy about strings is that strings should > > > be always put on one line. This makes sense since a typical use case > > > is for a user to type the error message into a search engine or > > > grep, and it won't be found if split across lines. This patch just > > > re-enables that check. > > > > > > Yes, lots of DPDK code now splits strings, that doesn't make it > > > right. > > > > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- > > > devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) > > > > > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > > > index a56c41a301c0..3e6081dd673e 100755 --- > > > a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 > > > +44,6 @@ options="$options --show-types" options="$options > > > --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ > > > VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ > > > PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ > > > -SPLIT_STRING,LONG_LINE_STRING,\ > > > LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ > > > NEW_TYPEDEFS,COMPARISON_TO_NULL" > > > > I'm not sure, given that the main reason for splitting strings in the > > first place is to avoid LONG_LINE_STRING warnings, I think we must > > choose between the two options. If split strings are not allowed, then > > long lines must be. > > > > Since checkpatches.sh is used by various automated scripts to complain > > loudly about problems in submissions, the above change prevents > > maintainers from writing long string at all (can't split and can't go > > past 80 columns). > > > > As a result, they will be tempted to cripple their code with nasty > > workarounds to shut up checkpatches.sh, we don't want that to happen. > > > > Also I think the reasons stated by original commit cf75514c8e2e are > > still relevant. My vote would be to keep things as is. > > > In my experience, checkpatch is smart enough to recognise when a long > line overflows the 80 character limit because of a single long string, > so the two options are not mutually exclusive. In other words, long > lines are not allowed except in the case where shortening the line > involves splitting a string. There may be a small amount of work in > getting checkpatch happy, i.e. by putting the string on a line on it's > own, but we can indeed have our cake and eat it too in this case. I can't seem to get around warnings without ignoring either SPLIT_STRING or LONG_LINE_STRING as of Linux v4.14-rc3's checkpatch.pl. I think you can only get around them by fooling it somehow. You really need to ignore at least LONG_LINE_STRING to meet the requirements of the commit log. However SPLIT_STRING still looks necessary to address part of cf75514c8e2e ("devtools: ignore warning on long log string"): "...lines that make use of PRIx64 with string concatenation will still be flagged if the beginning of the last string fragment begins after the 80 character threshold." It's not all that uncommon in my opinion. -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2017-10-02 16:21 ` Adrien Mazarguil @ 2017-10-03 10:38 ` Bruce Richardson 2017-10-03 10:56 ` Adrien Mazarguil 0 siblings, 1 reply; 11+ messages in thread From: Bruce Richardson @ 2017-10-03 10:38 UTC (permalink / raw) To: Adrien Mazarguil; +Cc: Stephen Hemminger, dev On Mon, Oct 02, 2017 at 06:21:06PM +0200, Adrien Mazarguil wrote: > On Mon, Oct 02, 2017 at 02:46:24PM +0100, Bruce Richardson wrote: > > On Mon, Oct 02, 2017 at 01:53:17PM +0200, Adrien Mazarguil wrote: > > > Hi Stephen, > > > > > > On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: > > > > The Linux kernel style policy about strings is that strings should > > > > be always put on one line. This makes sense since a typical use case > > > > is for a user to type the error message into a search engine or > > > > grep, and it won't be found if split across lines. This patch just > > > > re-enables that check. > > > > > > > > Yes, lots of DPDK code now splits strings, that doesn't make it > > > > right. > > > > > > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- > > > > devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) > > > > > > > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > > > > index a56c41a301c0..3e6081dd673e 100755 --- > > > > a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 > > > > +44,6 @@ options="$options --show-types" options="$options > > > > --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ > > > > VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ > > > > PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ > > > > -SPLIT_STRING,LONG_LINE_STRING,\ > > > > LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ > > > > NEW_TYPEDEFS,COMPARISON_TO_NULL" > > > > > > I'm not sure, given that the main reason for splitting strings in the > > > first place is to avoid LONG_LINE_STRING warnings, I think we must > > > choose between the two options. If split strings are not allowed, then > > > long lines must be. > > > > > > Since checkpatches.sh is used by various automated scripts to complain > > > loudly about problems in submissions, the above change prevents > > > maintainers from writing long string at all (can't split and can't go > > > past 80 columns). > > > > > > As a result, they will be tempted to cripple their code with nasty > > > workarounds to shut up checkpatches.sh, we don't want that to happen. > > > > > > Also I think the reasons stated by original commit cf75514c8e2e are > > > still relevant. My vote would be to keep things as is. > > > > > In my experience, checkpatch is smart enough to recognise when a long > > line overflows the 80 character limit because of a single long string, > > so the two options are not mutually exclusive. In other words, long > > lines are not allowed except in the case where shortening the line > > involves splitting a string. There may be a small amount of work in > > getting checkpatch happy, i.e. by putting the string on a line on it's > > own, but we can indeed have our cake and eat it too in this case. > > I can't seem to get around warnings without ignoring either SPLIT_STRING or > LONG_LINE_STRING as of Linux v4.14-rc3's checkpatch.pl. I think you can only > get around them by fooling it somehow. You really need to ignore at least > LONG_LINE_STRING to meet the requirements of the commit log. > > However SPLIT_STRING still looks necessary to address part of cf75514c8e2e > ("devtools: ignore warning on long log string"): > > "...lines that make use of PRIx64 with string concatenation will still be > flagged if the beginning of the last string fragment begins after the 80 > character threshold." > > It's not all that uncommon in my opinion. > If you have PRIx64 in it, it's not part of a literal string you would grep, so it's reasonable to split there. The user cannot know what the specific %x formatting character used is. /Bruce ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2017-10-03 10:38 ` Bruce Richardson @ 2017-10-03 10:56 ` Adrien Mazarguil 2019-03-28 19:02 ` Ferruh Yigit 0 siblings, 1 reply; 11+ messages in thread From: Adrien Mazarguil @ 2017-10-03 10:56 UTC (permalink / raw) To: Bruce Richardson; +Cc: Stephen Hemminger, dev On Tue, Oct 03, 2017 at 11:38:13AM +0100, Bruce Richardson wrote: > On Mon, Oct 02, 2017 at 06:21:06PM +0200, Adrien Mazarguil wrote: > > On Mon, Oct 02, 2017 at 02:46:24PM +0100, Bruce Richardson wrote: > > > On Mon, Oct 02, 2017 at 01:53:17PM +0200, Adrien Mazarguil wrote: > > > > Hi Stephen, > > > > > > > > On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: > > > > > The Linux kernel style policy about strings is that strings should > > > > > be always put on one line. This makes sense since a typical use case > > > > > is for a user to type the error message into a search engine or > > > > > grep, and it won't be found if split across lines. This patch just > > > > > re-enables that check. > > > > > > > > > > Yes, lots of DPDK code now splits strings, that doesn't make it > > > > > right. > > > > > > > > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- > > > > > devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) > > > > > > > > > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > > > > > index a56c41a301c0..3e6081dd673e 100755 --- > > > > > a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 > > > > > +44,6 @@ options="$options --show-types" options="$options > > > > > --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ > > > > > VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ > > > > > PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ > > > > > -SPLIT_STRING,LONG_LINE_STRING,\ > > > > > LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ > > > > > NEW_TYPEDEFS,COMPARISON_TO_NULL" > > > > > > > > I'm not sure, given that the main reason for splitting strings in the > > > > first place is to avoid LONG_LINE_STRING warnings, I think we must > > > > choose between the two options. If split strings are not allowed, then > > > > long lines must be. > > > > > > > > Since checkpatches.sh is used by various automated scripts to complain > > > > loudly about problems in submissions, the above change prevents > > > > maintainers from writing long string at all (can't split and can't go > > > > past 80 columns). > > > > > > > > As a result, they will be tempted to cripple their code with nasty > > > > workarounds to shut up checkpatches.sh, we don't want that to happen. > > > > > > > > Also I think the reasons stated by original commit cf75514c8e2e are > > > > still relevant. My vote would be to keep things as is. > > > > > > > In my experience, checkpatch is smart enough to recognise when a long > > > line overflows the 80 character limit because of a single long string, > > > so the two options are not mutually exclusive. In other words, long > > > lines are not allowed except in the case where shortening the line > > > involves splitting a string. There may be a small amount of work in > > > getting checkpatch happy, i.e. by putting the string on a line on it's > > > own, but we can indeed have our cake and eat it too in this case. > > > > I can't seem to get around warnings without ignoring either SPLIT_STRING or > > LONG_LINE_STRING as of Linux v4.14-rc3's checkpatch.pl. I think you can only > > get around them by fooling it somehow. You really need to ignore at least > > LONG_LINE_STRING to meet the requirements of the commit log. > > > > However SPLIT_STRING still looks necessary to address part of cf75514c8e2e > > ("devtools: ignore warning on long log string"): > > > > "...lines that make use of PRIx64 with string concatenation will still be > > flagged if the beginning of the last string fragment begins after the 80 > > character threshold." > > > > It's not all that uncommon in my opinion. > > > If you have PRIx64 in it, it's not part of a literal string you would > grep, so it's reasonable to split there. The user cannot know what the > specific %x formatting character used is. I agree, however in that case checkpatch would complain because our configuration doesn't specify to ignore SPLIT_STRING since there is no comma separator when concatenating them. My point is that the occasional exception is still necessary for split strings, that ignoring LONG_LINE_STRING must remain either way and unnecessary warnings cause more harm than good (they need to be worked around if we enforce this rule). In short, long/split strings acceptability assessment should be left to reviewers, as it cannot be automated in all cases through checkpatch.pl. -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2017-10-03 10:56 ` Adrien Mazarguil @ 2019-03-28 19:02 ` Ferruh Yigit 2019-03-28 19:02 ` Ferruh Yigit 2019-03-28 19:03 ` Ferruh Yigit 0 siblings, 2 replies; 11+ messages in thread From: Ferruh Yigit @ 2019-03-28 19:02 UTC (permalink / raw) To: Adrien Mazarguil Cc: Stephen Hemminger, Bruce Richardson, Luca Boccassi, Thomas Monjalon, dpdk-dev On 10/3/2017 11:56 AM, adrien.mazarguil at 6wind.com (Adrien Mazarguil) wrote: > On Tue, Oct 03, 2017 at 11:38:13AM +0100, Bruce Richardson wrote: >> On Mon, Oct 02, 2017 at 06:21:06PM +0200, Adrien Mazarguil wrote: >>> On Mon, Oct 02, 2017 at 02:46:24PM +0100, Bruce Richardson wrote: >>>> On Mon, Oct 02, 2017 at 01:53:17PM +0200, Adrien Mazarguil wrote: >>>>> Hi Stephen, >>>>> >>>>> On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: >>>>>> The Linux kernel style policy about strings is that strings should >>>>>> be always put on one line. This makes sense since a typical use case >>>>>> is for a user to type the error message into a search engine or >>>>>> grep, and it won't be found if split across lines. This patch just >>>>>> re-enables that check. >>>>>> >>>>>> Yes, lots of DPDK code now splits strings, that doesn't make it >>>>>> right. >>>>>> >>>>>> Signed-off-by: Stephen Hemminger <sthemmin at microsoft.com> --- >>>>>> devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) >>>>>> >>>>>> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh >>>>>> index a56c41a301c0..3e6081dd673e 100755 --- >>>>>> a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 >>>>>> +44,6 @@ options="$options --show-types" options="$options >>>>>> --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ >>>>>> VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ >>>>>> PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ >>>>>> -SPLIT_STRING,LONG_LINE_STRING,\ >>>>>> LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ >>>>>> NEW_TYPEDEFS,COMPARISON_TO_NULL" >>>>> >>>>> I'm not sure, given that the main reason for splitting strings in the >>>>> first place is to avoid LONG_LINE_STRING warnings, I think we must >>>>> choose between the two options. If split strings are not allowed, then >>>>> long lines must be. >>>>> >>>>> Since checkpatches.sh is used by various automated scripts to complain >>>>> loudly about problems in submissions, the above change prevents >>>>> maintainers from writing long string at all (can't split and can't go >>>>> past 80 columns). >>>>> >>>>> As a result, they will be tempted to cripple their code with nasty >>>>> workarounds to shut up checkpatches.sh, we don't want that to happen. >>>>> >>>>> Also I think the reasons stated by original commit cf75514c8e2e are >>>>> still relevant. My vote would be to keep things as is. >>>>> >>>> In my experience, checkpatch is smart enough to recognise when a long >>>> line overflows the 80 character limit because of a single long string, >>>> so the two options are not mutually exclusive. In other words, long >>>> lines are not allowed except in the case where shortening the line >>>> involves splitting a string. There may be a small amount of work in >>>> getting checkpatch happy, i.e. by putting the string on a line on it's >>>> own, but we can indeed have our cake and eat it too in this case. >>> >>> I can't seem to get around warnings without ignoring either SPLIT_STRING or >>> LONG_LINE_STRING as of Linux v4.14-rc3's checkpatch.pl. I think you can only >>> get around them by fooling it somehow. You really need to ignore at least >>> LONG_LINE_STRING to meet the requirements of the commit log. >>> >>> However SPLIT_STRING still looks necessary to address part of cf75514c8e2e >>> ("devtools: ignore warning on long log string"): >>> >>> "...lines that make use of PRIx64 with string concatenation will still be >>> flagged if the beginning of the last string fragment begins after the 80 >>> character threshold." >>> >>> It's not all that uncommon in my opinion. >>> >> If you have PRIx64 in it, it's not part of a literal string you would >> grep, so it's reasonable to split there. The user cannot know what the >> specific %x formatting character used is. > > I agree, however in that case checkpatch would complain because our > configuration doesn't specify to ignore SPLIT_STRING since there is no comma > separator when concatenating them. > > My point is that the occasional exception is still necessary for split > strings, that ignoring LONG_LINE_STRING must remain either way and > unnecessary warnings cause more harm than good (they need to be worked > around if we enforce this rule). > > In short, long/split strings acceptability assessment should be left to > reviewers, as it cannot be automated in all cases through checkpatch.pl. > This patch is waiting in patchwork for a long time now. My experience is same with Adrien's, if 'LONG_LINE_STRING' is not ignored, it will complain about long log messages, so removing 'LONG_LINE_STRING' contradicts with the reason of the patch described in the commit log. Perhaps it can be an option to remove only 'SPLIT_STRING' from ignore list, to detect split messages. But overall, I am updating this patch as "Change Requested", if there is a demand for ignoring 'SPLIT_STRING' please send a new version. Thanks, ferruh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2019-03-28 19:02 ` Ferruh Yigit @ 2019-03-28 19:02 ` Ferruh Yigit 2019-03-28 19:03 ` Ferruh Yigit 1 sibling, 0 replies; 11+ messages in thread From: Ferruh Yigit @ 2019-03-28 19:02 UTC (permalink / raw) To: Adrien Mazarguil Cc: Stephen Hemminger, Bruce Richardson, Luca Boccassi, Thomas Monjalon, dpdk-dev On 10/3/2017 11:56 AM, adrien.mazarguil at 6wind.com (Adrien Mazarguil) wrote: > On Tue, Oct 03, 2017 at 11:38:13AM +0100, Bruce Richardson wrote: >> On Mon, Oct 02, 2017 at 06:21:06PM +0200, Adrien Mazarguil wrote: >>> On Mon, Oct 02, 2017 at 02:46:24PM +0100, Bruce Richardson wrote: >>>> On Mon, Oct 02, 2017 at 01:53:17PM +0200, Adrien Mazarguil wrote: >>>>> Hi Stephen, >>>>> >>>>> On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: >>>>>> The Linux kernel style policy about strings is that strings should >>>>>> be always put on one line. This makes sense since a typical use case >>>>>> is for a user to type the error message into a search engine or >>>>>> grep, and it won't be found if split across lines. This patch just >>>>>> re-enables that check. >>>>>> >>>>>> Yes, lots of DPDK code now splits strings, that doesn't make it >>>>>> right. >>>>>> >>>>>> Signed-off-by: Stephen Hemminger <sthemmin at microsoft.com> --- >>>>>> devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) >>>>>> >>>>>> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh >>>>>> index a56c41a301c0..3e6081dd673e 100755 --- >>>>>> a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 >>>>>> +44,6 @@ options="$options --show-types" options="$options >>>>>> --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ >>>>>> VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ >>>>>> PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ >>>>>> -SPLIT_STRING,LONG_LINE_STRING,\ >>>>>> LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ >>>>>> NEW_TYPEDEFS,COMPARISON_TO_NULL" >>>>> >>>>> I'm not sure, given that the main reason for splitting strings in the >>>>> first place is to avoid LONG_LINE_STRING warnings, I think we must >>>>> choose between the two options. If split strings are not allowed, then >>>>> long lines must be. >>>>> >>>>> Since checkpatches.sh is used by various automated scripts to complain >>>>> loudly about problems in submissions, the above change prevents >>>>> maintainers from writing long string at all (can't split and can't go >>>>> past 80 columns). >>>>> >>>>> As a result, they will be tempted to cripple their code with nasty >>>>> workarounds to shut up checkpatches.sh, we don't want that to happen. >>>>> >>>>> Also I think the reasons stated by original commit cf75514c8e2e are >>>>> still relevant. My vote would be to keep things as is. >>>>> >>>> In my experience, checkpatch is smart enough to recognise when a long >>>> line overflows the 80 character limit because of a single long string, >>>> so the two options are not mutually exclusive. In other words, long >>>> lines are not allowed except in the case where shortening the line >>>> involves splitting a string. There may be a small amount of work in >>>> getting checkpatch happy, i.e. by putting the string on a line on it's >>>> own, but we can indeed have our cake and eat it too in this case. >>> >>> I can't seem to get around warnings without ignoring either SPLIT_STRING or >>> LONG_LINE_STRING as of Linux v4.14-rc3's checkpatch.pl. I think you can only >>> get around them by fooling it somehow. You really need to ignore at least >>> LONG_LINE_STRING to meet the requirements of the commit log. >>> >>> However SPLIT_STRING still looks necessary to address part of cf75514c8e2e >>> ("devtools: ignore warning on long log string"): >>> >>> "...lines that make use of PRIx64 with string concatenation will still be >>> flagged if the beginning of the last string fragment begins after the 80 >>> character threshold." >>> >>> It's not all that uncommon in my opinion. >>> >> If you have PRIx64 in it, it's not part of a literal string you would >> grep, so it's reasonable to split there. The user cannot know what the >> specific %x formatting character used is. > > I agree, however in that case checkpatch would complain because our > configuration doesn't specify to ignore SPLIT_STRING since there is no comma > separator when concatenating them. > > My point is that the occasional exception is still necessary for split > strings, that ignoring LONG_LINE_STRING must remain either way and > unnecessary warnings cause more harm than good (they need to be worked > around if we enforce this rule). > > In short, long/split strings acceptability assessment should be left to > reviewers, as it cannot be automated in all cases through checkpatch.pl. > This patch is waiting in patchwork for a long time now. My experience is same with Adrien's, if 'LONG_LINE_STRING' is not ignored, it will complain about long log messages, so removing 'LONG_LINE_STRING' contradicts with the reason of the patch described in the commit log. Perhaps it can be an option to remove only 'SPLIT_STRING' from ignore list, to detect split messages. But overall, I am updating this patch as "Change Requested", if there is a demand for ignoring 'SPLIT_STRING' please send a new version. Thanks, ferruh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2019-03-28 19:02 ` Ferruh Yigit 2019-03-28 19:02 ` Ferruh Yigit @ 2019-03-28 19:03 ` Ferruh Yigit 2019-03-28 19:03 ` Ferruh Yigit 1 sibling, 1 reply; 11+ messages in thread From: Ferruh Yigit @ 2019-03-28 19:03 UTC (permalink / raw) To: Adrien Mazarguil Cc: Stephen Hemminger, Bruce Richardson, Luca Boccassi, Thomas Monjalon, dpdk-dev On 3/28/2019 7:02 PM, Ferruh Yigit wrote: > On 10/3/2017 11:56 AM, adrien.mazarguil at 6wind.com (Adrien Mazarguil) wrote: >> On Tue, Oct 03, 2017 at 11:38:13AM +0100, Bruce Richardson wrote: >>> On Mon, Oct 02, 2017 at 06:21:06PM +0200, Adrien Mazarguil wrote: >>>> On Mon, Oct 02, 2017 at 02:46:24PM +0100, Bruce Richardson wrote: >>>>> On Mon, Oct 02, 2017 at 01:53:17PM +0200, Adrien Mazarguil wrote: >>>>>> Hi Stephen, >>>>>> >>>>>> On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: >>>>>>> The Linux kernel style policy about strings is that strings should >>>>>>> be always put on one line. This makes sense since a typical use case >>>>>>> is for a user to type the error message into a search engine or >>>>>>> grep, and it won't be found if split across lines. This patch just >>>>>>> re-enables that check. >>>>>>> >>>>>>> Yes, lots of DPDK code now splits strings, that doesn't make it >>>>>>> right. >>>>>>> >>>>>>> Signed-off-by: Stephen Hemminger <sthemmin at microsoft.com> --- >>>>>>> devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) >>>>>>> >>>>>>> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh >>>>>>> index a56c41a301c0..3e6081dd673e 100755 --- >>>>>>> a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 >>>>>>> +44,6 @@ options="$options --show-types" options="$options >>>>>>> --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ >>>>>>> VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ >>>>>>> PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ >>>>>>> -SPLIT_STRING,LONG_LINE_STRING,\ >>>>>>> LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ >>>>>>> NEW_TYPEDEFS,COMPARISON_TO_NULL" >>>>>> >>>>>> I'm not sure, given that the main reason for splitting strings in the >>>>>> first place is to avoid LONG_LINE_STRING warnings, I think we must >>>>>> choose between the two options. If split strings are not allowed, then >>>>>> long lines must be. >>>>>> >>>>>> Since checkpatches.sh is used by various automated scripts to complain >>>>>> loudly about problems in submissions, the above change prevents >>>>>> maintainers from writing long string at all (can't split and can't go >>>>>> past 80 columns). >>>>>> >>>>>> As a result, they will be tempted to cripple their code with nasty >>>>>> workarounds to shut up checkpatches.sh, we don't want that to happen. >>>>>> >>>>>> Also I think the reasons stated by original commit cf75514c8e2e are >>>>>> still relevant. My vote would be to keep things as is. >>>>>> >>>>> In my experience, checkpatch is smart enough to recognise when a long >>>>> line overflows the 80 character limit because of a single long string, >>>>> so the two options are not mutually exclusive. In other words, long >>>>> lines are not allowed except in the case where shortening the line >>>>> involves splitting a string. There may be a small amount of work in >>>>> getting checkpatch happy, i.e. by putting the string on a line on it's >>>>> own, but we can indeed have our cake and eat it too in this case. >>>> >>>> I can't seem to get around warnings without ignoring either SPLIT_STRING or >>>> LONG_LINE_STRING as of Linux v4.14-rc3's checkpatch.pl. I think you can only >>>> get around them by fooling it somehow. You really need to ignore at least >>>> LONG_LINE_STRING to meet the requirements of the commit log. >>>> >>>> However SPLIT_STRING still looks necessary to address part of cf75514c8e2e >>>> ("devtools: ignore warning on long log string"): >>>> >>>> "...lines that make use of PRIx64 with string concatenation will still be >>>> flagged if the beginning of the last string fragment begins after the 80 >>>> character threshold." >>>> >>>> It's not all that uncommon in my opinion. >>>> >>> If you have PRIx64 in it, it's not part of a literal string you would >>> grep, so it's reasonable to split there. The user cannot know what the >>> specific %x formatting character used is. >> >> I agree, however in that case checkpatch would complain because our >> configuration doesn't specify to ignore SPLIT_STRING since there is no comma >> separator when concatenating them. >> >> My point is that the occasional exception is still necessary for split >> strings, that ignoring LONG_LINE_STRING must remain either way and >> unnecessary warnings cause more harm than good (they need to be worked >> around if we enforce this rule). >> >> In short, long/split strings acceptability assessment should be left to >> reviewers, as it cannot be automated in all cases through checkpatch.pl. >> > > This patch is waiting in patchwork for a long time now. > > My experience is same with Adrien's, if 'LONG_LINE_STRING' is not ignored, it > will complain about long log messages, so removing 'LONG_LINE_STRING' > contradicts with the reason of the patch described in the commit log. > > Perhaps it can be an option to remove only 'SPLIT_STRING' from ignore list, to > detect split messages. > > But overall, I am updating this patch as "Change Requested", if there is a > demand for ignoring 'SPLIT_STRING' please send a new version. > Sorry for the noise, I keep forgetting this, for reference the patch mentioned: https://patches.dpdk.org/patch/29438/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings 2019-03-28 19:03 ` Ferruh Yigit @ 2019-03-28 19:03 ` Ferruh Yigit 0 siblings, 0 replies; 11+ messages in thread From: Ferruh Yigit @ 2019-03-28 19:03 UTC (permalink / raw) To: Adrien Mazarguil Cc: Stephen Hemminger, Bruce Richardson, Luca Boccassi, Thomas Monjalon, dpdk-dev On 3/28/2019 7:02 PM, Ferruh Yigit wrote: > On 10/3/2017 11:56 AM, adrien.mazarguil at 6wind.com (Adrien Mazarguil) wrote: >> On Tue, Oct 03, 2017 at 11:38:13AM +0100, Bruce Richardson wrote: >>> On Mon, Oct 02, 2017 at 06:21:06PM +0200, Adrien Mazarguil wrote: >>>> On Mon, Oct 02, 2017 at 02:46:24PM +0100, Bruce Richardson wrote: >>>>> On Mon, Oct 02, 2017 at 01:53:17PM +0200, Adrien Mazarguil wrote: >>>>>> Hi Stephen, >>>>>> >>>>>> On Fri, Sep 29, 2017 at 08:37:49AM -0700, Stephen Hemminger wrote: >>>>>>> The Linux kernel style policy about strings is that strings should >>>>>>> be always put on one line. This makes sense since a typical use case >>>>>>> is for a user to type the error message into a search engine or >>>>>>> grep, and it won't be found if split across lines. This patch just >>>>>>> re-enables that check. >>>>>>> >>>>>>> Yes, lots of DPDK code now splits strings, that doesn't make it >>>>>>> right. >>>>>>> >>>>>>> Signed-off-by: Stephen Hemminger <sthemmin at microsoft.com> --- >>>>>>> devtools/checkpatches.sh | 1 - 1 file changed, 1 deletion(-) >>>>>>> >>>>>>> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh >>>>>>> index a56c41a301c0..3e6081dd673e 100755 --- >>>>>>> a/devtools/checkpatches.sh +++ b/devtools/checkpatches.sh @@ -44,7 >>>>>>> +44,6 @@ options="$options --show-types" options="$options >>>>>>> --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\ >>>>>>> VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\ >>>>>>> PREFER_KERNEL_TYPES,BIT_MACRO,CONST_STRUCT,\ >>>>>>> -SPLIT_STRING,LONG_LINE_STRING,\ >>>>>>> LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ >>>>>>> NEW_TYPEDEFS,COMPARISON_TO_NULL" >>>>>> >>>>>> I'm not sure, given that the main reason for splitting strings in the >>>>>> first place is to avoid LONG_LINE_STRING warnings, I think we must >>>>>> choose between the two options. If split strings are not allowed, then >>>>>> long lines must be. >>>>>> >>>>>> Since checkpatches.sh is used by various automated scripts to complain >>>>>> loudly about problems in submissions, the above change prevents >>>>>> maintainers from writing long string at all (can't split and can't go >>>>>> past 80 columns). >>>>>> >>>>>> As a result, they will be tempted to cripple their code with nasty >>>>>> workarounds to shut up checkpatches.sh, we don't want that to happen. >>>>>> >>>>>> Also I think the reasons stated by original commit cf75514c8e2e are >>>>>> still relevant. My vote would be to keep things as is. >>>>>> >>>>> In my experience, checkpatch is smart enough to recognise when a long >>>>> line overflows the 80 character limit because of a single long string, >>>>> so the two options are not mutually exclusive. In other words, long >>>>> lines are not allowed except in the case where shortening the line >>>>> involves splitting a string. There may be a small amount of work in >>>>> getting checkpatch happy, i.e. by putting the string on a line on it's >>>>> own, but we can indeed have our cake and eat it too in this case. >>>> >>>> I can't seem to get around warnings without ignoring either SPLIT_STRING or >>>> LONG_LINE_STRING as of Linux v4.14-rc3's checkpatch.pl. I think you can only >>>> get around them by fooling it somehow. You really need to ignore at least >>>> LONG_LINE_STRING to meet the requirements of the commit log. >>>> >>>> However SPLIT_STRING still looks necessary to address part of cf75514c8e2e >>>> ("devtools: ignore warning on long log string"): >>>> >>>> "...lines that make use of PRIx64 with string concatenation will still be >>>> flagged if the beginning of the last string fragment begins after the 80 >>>> character threshold." >>>> >>>> It's not all that uncommon in my opinion. >>>> >>> If you have PRIx64 in it, it's not part of a literal string you would >>> grep, so it's reasonable to split there. The user cannot know what the >>> specific %x formatting character used is. >> >> I agree, however in that case checkpatch would complain because our >> configuration doesn't specify to ignore SPLIT_STRING since there is no comma >> separator when concatenating them. >> >> My point is that the occasional exception is still necessary for split >> strings, that ignoring LONG_LINE_STRING must remain either way and >> unnecessary warnings cause more harm than good (they need to be worked >> around if we enforce this rule). >> >> In short, long/split strings acceptability assessment should be left to >> reviewers, as it cannot be automated in all cases through checkpatch.pl. >> > > This patch is waiting in patchwork for a long time now. > > My experience is same with Adrien's, if 'LONG_LINE_STRING' is not ignored, it > will complain about long log messages, so removing 'LONG_LINE_STRING' > contradicts with the reason of the patch described in the commit log. > > Perhaps it can be an option to remove only 'SPLIT_STRING' from ignore list, to > detect split messages. > > But overall, I am updating this patch as "Change Requested", if there is a > demand for ignoring 'SPLIT_STRING' please send a new version. > Sorry for the noise, I keep forgetting this, for reference the patch mentioned: https://patches.dpdk.org/patch/29438/ ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-03-28 19:03 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-29 15:37 [dpdk-dev] [PATCH] checkpatch: re-enable warnings about split long strings Stephen Hemminger 2017-10-02 10:01 ` Luca Boccassi 2017-10-02 11:53 ` Adrien Mazarguil 2017-10-02 13:46 ` Bruce Richardson 2017-10-02 16:21 ` Adrien Mazarguil 2017-10-03 10:38 ` Bruce Richardson 2017-10-03 10:56 ` Adrien Mazarguil 2019-03-28 19:02 ` Ferruh Yigit 2019-03-28 19:02 ` Ferruh Yigit 2019-03-28 19:03 ` Ferruh Yigit 2019-03-28 19:03 ` Ferruh Yigit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).