diff mbox series

[v3] Documentation/process/coding-style.rst: space around const

Message ID 20231010125832.1002941-1-max.kellermann@ionos.com
State New
Headers
Series [v3] Documentation/process/coding-style.rst: space around const |

Commit Message

Max Kellermann Oct. 10, 2023, 12:58 p.m. UTC 
  There are currently no rules on the placement of "const", but a recent
code submission revealed that there is clearly a preference for spaces
around it.

checkpatch.pl has no check at all for this; though it does sometimes
complain, but only because it erroneously thinks that the "*" (on
local variables) is an unary dereference operator, not a pointer type.

Current coding style for const pointers-to-pointers:

 "*const*": 2 occurrences
 "* const*": 3
 "*const *": 182
 "* const *": 681

Just const pointers:

 "*const": 2833 occurrences
 "* const": 16615

Link: https://lore.kernel.org/r/264fa39d-aed6-4a54-a085-107997078f8d@roeck-us.net/
Link: https://lore.kernel.org/r/f511170fe61d7e7214a3a062661cf4103980dad6.camel@perches.com/
Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
---
V1 -> V2: removed "volatile" on gregkh's request.
V2 -> V3: moved patch changelog below the "---" line
---
 Documentation/process/coding-style.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Greg KH Oct. 10, 2023, 7:24 p.m. UTC | #1 
On Tue, Oct 10, 2023 at 02:58:31PM +0200, Max Kellermann wrote:
> There are currently no rules on the placement of "const", but a recent
> code submission revealed that there is clearly a preference for spaces
> around it.
> 
> checkpatch.pl has no check at all for this; though it does sometimes
> complain, but only because it erroneously thinks that the "*" (on
> local variables) is an unary dereference operator, not a pointer type.
> 
> Current coding style for const pointers-to-pointers:
> 
>  "*const*": 2 occurrences
>  "* const*": 3
>  "*const *": 182
>  "* const *": 681
> 
> Just const pointers:
> 
>  "*const": 2833 occurrences
>  "* const": 16615
> 
> Link: https://lore.kernel.org/r/264fa39d-aed6-4a54-a085-107997078f8d@roeck-us.net/
> Link: https://lore.kernel.org/r/f511170fe61d7e7214a3a062661cf4103980dad6.camel@perches.com/
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> ---
> V1 -> V2: removed "volatile" on gregkh's request.
> V2 -> V3: moved patch changelog below the "---" line
> ---
>  Documentation/process/coding-style.rst | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 6db37a46d305..71d62d81e506 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -271,6 +271,17 @@ adjacent to the type name.  Examples:
>  	unsigned long long memparse(char *ptr, char **retptr);
>  	char *match_strdup(substring_t *s);
>  
> +Use space around the ``const`` keyword (except when adjacent to
> +parentheses).  Example:
> +
> +.. code-block:: c
> +
> +	const void *a;
> +	void * const b;
> +	void ** const c;
> +	void * const * const d;
> +	int strcmp(const char *a, const char *b);
> +
>  Use one space around (on each side of) most binary and ternary operators,
>  such as any of these::
>  
> -- 
> 2.39.2
> 

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Guenter Roeck Oct. 10, 2023, 11:46 p.m. UTC | #2 
On Tue, Oct 10, 2023 at 02:58:31PM +0200, Max Kellermann wrote:
> There are currently no rules on the placement of "const", but a recent
> code submission revealed that there is clearly a preference for spaces
> around it.
> 
> checkpatch.pl has no check at all for this; though it does sometimes
> complain, but only because it erroneously thinks that the "*" (on
> local variables) is an unary dereference operator, not a pointer type.
> 
> Current coding style for const pointers-to-pointers:
> 
>  "*const*": 2 occurrences
>  "* const*": 3
>  "*const *": 182
>  "* const *": 681
> 
> Just const pointers:
> 
>  "*const": 2833 occurrences
>  "* const": 16615
> 
> Link: https://lore.kernel.org/r/264fa39d-aed6-4a54-a085-107997078f8d@roeck-us.net/
> Link: https://lore.kernel.org/r/f511170fe61d7e7214a3a062661cf4103980dad6.camel@perches.com/
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> V1 -> V2: removed "volatile" on gregkh's request.
> V2 -> V3: moved patch changelog below the "---" line
> ---
>  Documentation/process/coding-style.rst | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 6db37a46d305..71d62d81e506 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -271,6 +271,17 @@ adjacent to the type name.  Examples:
>  	unsigned long long memparse(char *ptr, char **retptr);
>  	char *match_strdup(substring_t *s);
>  
> +Use space around the ``const`` keyword (except when adjacent to
> +parentheses).  Example:
> +
> +.. code-block:: c
> +
> +	const void *a;
> +	void * const b;
> +	void ** const c;
> +	void * const * const d;
> +	int strcmp(const char *a, const char *b);
> +
>  Use one space around (on each side of) most binary and ternary operators,
>  such as any of these::
>  
> -- 
> 2.39.2
>
Joe Perches Oct. 11, 2023, 12:45 a.m. UTC | #3 
On Tue, 2023-10-10 at 14:58 +0200, Max Kellermann wrote:
> There are currently no rules on the placement of "const", but a recent
> code submission revealed that there is clearly a preference for spaces
> around it.
> 
> checkpatch.pl has no check at all for this; though it does sometimes
> complain, but only because it erroneously thinks that the "*" (on
> local variables) is an unary dereference operator, not a pointer type.
> 

Maybe something like this for checkpatch:
---
 scripts/checkpatch.pl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25fdb7fda1128..48d70d0ad9a2b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4726,6 +4726,16 @@ sub process {
 			}
 		}
 
+# check for const* and *const uses that should have space around const
+		if ($line =~ /(?:const\*|\*const)/) {
+			if (WARN("CONST_POINTER",
+				 "const pointers should have spaces around const\n" . $herecurr) &&
+			    $fix) {
+				$fixed[$fixlinenr] =~ s/\*const\b/* const/g;
+				$fixed[$fixlinenr] =~ s/\bconst\*/const */g;
+			}
+		}
+
 # check for non-global char *foo[] = {"bar", ...} declarations.
 		if ($line =~ /^.\s+(?:static\s+|const\s+)?char\s+\*\s*\w+\s*\[\s*\]\s*=\s*\{/) {
 			WARN("STATIC_CONST_CHAR_ARRAY",
Dan Williams Oct. 11, 2023, 9:44 p.m. UTC | #4 
Max Kellermann wrote:
> There are currently no rules on the placement of "const", but a recent
> code submission revealed that there is clearly a preference for spaces
> around it.
> 
> checkpatch.pl has no check at all for this; though it does sometimes
> complain, but only because it erroneously thinks that the "*" (on
> local variables) is an unary dereference operator, not a pointer type.
> 
> Current coding style for const pointers-to-pointers:
> 
>  "*const*": 2 occurrences
>  "* const*": 3
>  "*const *": 182
>  "* const *": 681
> 
> Just const pointers:
> 
>  "*const": 2833 occurrences
>  "* const": 16615
> 
> Link: https://lore.kernel.org/r/264fa39d-aed6-4a54-a085-107997078f8d@roeck-us.net/
> Link: https://lore.kernel.org/r/f511170fe61d7e7214a3a062661cf4103980dad6.camel@perches.com/
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> ---
> V1 -> V2: removed "volatile" on gregkh's request.
> V2 -> V3: moved patch changelog below the "---" line
> ---
>  Documentation/process/coding-style.rst | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 6db37a46d305..71d62d81e506 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -271,6 +271,17 @@ adjacent to the type name.  Examples:
>  	unsigned long long memparse(char *ptr, char **retptr);
>  	char *match_strdup(substring_t *s);
>  
> +Use space around the ``const`` keyword (except when adjacent to
> +parentheses).  Example:
> +
> +.. code-block:: c
> +
> +	const void *a;
> +	void * const b;
> +	void ** const c;
> +	void * const * const d;
> +	int strcmp(const char *a, const char *b);
> +
>  Use one space around (on each side of) most binary and ternary operators,
>  such as any of these::

I notice that clang-format reflows that example to:

     const void *a;
     void *const b;
     void **const c;
     void *const *const d;
     int strcmp(const char *a, const char *b);

...but someone more clang-format savvy than me would need to propose the
changes to the kernel's .clang-format template to match the style
suggestion.
Miguel Ojeda Oct. 12, 2023, 11:50 a.m. UTC | #5 
On Wed, 11 Oct 2023 14:44:17 -0700, Dan Williams wrote:
>
> I notice that clang-format reflows that example to:
>
>     const void *a;
>     void *const b;
>     void **const c;
>     void *const *const d;
>     int strcmp(const char *a, const char *b);
>
> ...but someone more clang-format savvy than me would need to propose the
> changes to the kernel's .clang-format template to match the style
> suggestion.

I think we could use:

    diff --git a/.clang-format b/.clang-format
    index 0bbb1991defe..9eeb511c0814 100644
    --- a/.clang-format
    +++ b/.clang-format
    @@ -671,6 +671,7 @@ SortIncludes: false
     SortUsingDeclarations: false
     SpaceAfterCStyleCast: false
     SpaceAfterTemplateKeyword: true
    +SpaceAroundPointerQualifiers: Both
     SpaceBeforeAssignmentOperators: true
     SpaceBeforeCtorInitializerColon: true
     SpaceBeforeInheritanceColon: true

At least that makes it match the documentation example -- I got this:

    const void *a;
    void * const b;
    void ** const c;
    void * const * const d;
    int strcmp(const char *a, const char *b);

But it is only supported in version >= 12, so we need to wait for the
minimum LLVM version bump.

(Thanks for the ping, Joe!)

Cheers,
Miguel
Joe Perches Oct. 12, 2023, 2:48 p.m. UTC | #6 
On Thu, 2023-10-12 at 13:50 +0200, Miguel Ojeda wrote:
> On Wed, 11 Oct 2023 14:44:17 -0700, Dan Williams wrote:
> > 
> > I notice that clang-format reflows that example to:
> > 
> >     const void *a;
> >     void *const b;
> >     void **const c;
> >     void *const *const d;
> >     int strcmp(const char *a, const char *b);
> > 
> > ...but someone more clang-format savvy than me would need to propose the
> > changes to the kernel's .clang-format template to match the style
> > suggestion.
> 
> I think we could use:
> 
>     diff --git a/.clang-format b/.clang-format
>     index 0bbb1991defe..9eeb511c0814 100644
>     --- a/.clang-format
>     +++ b/.clang-format
>     @@ -671,6 +671,7 @@ SortIncludes: false
>      SortUsingDeclarations: false
>      SpaceAfterCStyleCast: false
>      SpaceAfterTemplateKeyword: true
>     +SpaceAroundPointerQualifiers: Both
>      SpaceBeforeAssignmentOperators: true
>      SpaceBeforeCtorInitializerColon: true
>      SpaceBeforeInheritanceColon: true
> 
> At least that makes it match the documentation example -- I got this:
> 
>     const void *a;
>     void * const b;
>     void ** const c;
>     void * const * const d;
>     int strcmp(const char *a, const char *b);
> 
> But it is only supported in version >= 12, so we need to wait for the
> minimum LLVM version bump.

Do older versions of clang-format ignore entries
they don't understand?
Miguel Ojeda Oct. 12, 2023, 4:53 p.m. UTC | #7 
On Thu, Oct 12, 2023 at 4:48 PM Joe Perches <joe@perches.com> wrote:
>
> Do older versions of clang-format ignore entries
> they don't understand?

Sadly, no, that is the reason we keep it at the minimum.

However, I just took a look again at it, and I see that such support
was added to LLVM 12, the `--Wno-error=unknown` flag in commit
f64903fd8176 ("Add -Wno-error=unknown flag to clang-format.").

So this means that the minimum is bumped to 12, we could in principle
use newer options.

I think the downsides are that users will need to pass the flag
(potentially in e.g. their IDE or similar) and that formatting could
be potentially chaotic depending on the options ignored. I guess
particular subsystems could agree on which version to use.

Cheers,
Miguel
diff mbox series

Patch

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 6db37a46d305..71d62d81e506 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -271,6 +271,17 @@  adjacent to the type name.  Examples:
 	unsigned long long memparse(char *ptr, char **retptr);
 	char *match_strdup(substring_t *s);
 
+Use space around the ``const`` keyword (except when adjacent to
+parentheses).  Example:
+
+.. code-block:: c
+
+	const void *a;
+	void * const b;
+	void ** const c;
+	void * const * const d;
+	int strcmp(const char *a, const char *b);
+
 Use one space around (on each side of) most binary and ternary operators,
 such as any of these::