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/[email protected]/
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Max Kellermann <[email protected]>
---
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
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/[email protected]/
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Max Kellermann <[email protected]>
> ---
> 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 <[email protected]>
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/[email protected]/
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Max Kellermann <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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
>
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",
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/[email protected]/
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Max Kellermann <[email protected]>
> ---
> 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.
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
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?
On Thu, Oct 12, 2023 at 4:48 PM Joe Perches <[email protected]> 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