[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> The reason for me to work on this feature is the fact that clang-format 
> doesn't work reliable for changing the location of the const. After using 
> clang-tidy and clang-format some consts were still one the right hand side

Its true we made the decision if clang-format couldn't work it out we'd bail 
out and NOT make the change. We felt it was better to be correct than change 
all the const's

but if you could show us the examples it might help us work through those cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp:43
+  // of the `*`.
+  int *IPtr = 
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'IPtr' of type 'int *' 
can be declared 'const'

could you please provide more test cases with pointers in combination with

- macros
- typedefs
- template parameters
- `auto *`

I think we should be thorough here. The users might not have all details of 
where `const` applies in their forehead and big transformations of code-bases 
should definitely not break the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

during the original implementation of the fix-util I had the plan to provide an 
option for east-vs-west const, but during the discussions we came to the 
conclusion that `clang-format` should do this (the patches were already pending 
at that point in time).

if this option works, i think its ok to provide it. having this check on during 
development is maybe just annoying when the `const` always pops up at the 
"wrong" side.

but i think we should provide more test cases here, especially for the edge 
cases where `const` _must_ be on the right side, because it would otherwise 
apply to something else then intended. that was the original reason why i 
sticked to east-const, because thats always correct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: owenpan, MyDeveloperDay.
aaron.ballman added a comment.

In D131386#3723490 , @Mordante wrote:

> In D131386#3722749 , @aaron.ballman 
> wrote:
>
>> We leave formatting decisions in clang-tidy to clang-format and I don't 
>> think we should deviate from that policy here without a very clear 
>> understanding of when we should relax that restriction. That said, I'm 
>> personally not certain we should have such an option (the long-term goal has 
>> generally been to integrate clang-format functionality into clang-tidy so 
>> there can be an option to just run format after applying fixes in a TU). Is 
>> there a compelling reason we should have it?
>
> The reason for me to work on this feature is the fact that clang-format 
> doesn't work reliable for changing the location of the `const`. After using 
> clang-tidy and clang-format some `const`s were still one the right hand side. 
> I didn't do a deep investigation, but it seems to affect classes in 
> namespaces. So some occurrences of `std::string` were affected, but not all. 
> So in the current state I can't use this clang-tidy fix.

CC @MyDeveloperDay and @owenpan for awareness.

> Since clang-format doesn't fully parse the code I wonder whether all cases 
> can be properly fixed. On the other hand clang-tidy has all information 
> available.

Mostly. clang-tidy will miss everything in a preprocessor conditional block, as 
an example of what it can't handle but clang-format can.

> Would integrating clang-format in clang-tidy mean clang-format will use the 
> full information of the AST?

I wouldn't imagine that to be the case (that'd be a pretty big change to the 
architecture of clang-format; basically a rewrite). I would expect it to be 
integrated more as a library to call into to perform the post-fix formatting to 
just specific ranges of code. However, it is interesting to think about the 
fact that such a library could potentially accept an optional AST node 
representing the root of the AST represented by the text as a way to help it 
disambiguate situations it couldn't otherwise figure out.

In D131386#3723254 , @carlosgalvezp 
wrote:

> FWIW, there are already other checks that have formatting settings, for 
> example:
> https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-bounds-constant-array-index.html#cmdoption-arg-includestyle

Those docs are pretty terrible because I have no idea what an "llvm-style" 
include even *is*.

IIRC, we added this because we added the include sorter to clang-tidy and at 
the time, it was deemed unsafe to add to clang-format because of the likelihood 
of breaking code. IMO, it's on the borderline as to whether we should have 
added it or not -- it's the same situation I was worried about above where we 
add a feature to clang-tidy and then clang-format later gets the ability to 
perform the formatting and so the two gradually drift out of sync. Then again, 
it seems our checks have drifted out of sync with what IncludeSorter supports 
because there's a third option for google Objective C formatting style 
(whatever that is).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

In D131386#3722749 , @aaron.ballman 
wrote:

> We leave formatting decisions in clang-tidy to clang-format and I don't think 
> we should deviate from that policy here without a very clear understanding of 
> when we should relax that restriction. That said, I'm personally not certain 
> we should have such an option (the long-term goal has generally been to 
> integrate clang-format functionality into clang-tidy so there can be an 
> option to just run format after applying fixes in a TU). Is there a 
> compelling reason we should have it?

The reason for me to work on this feature is the fact that clang-format doesn't 
work reliable for changing the location of the `const`. After using clang-tidy 
and clang-format some `const`s were still one the right hand side. I didn't do 
a deep investigation, but it seems to affect classes in namespaces. So some 
occurrences of `std::string` were affected, but not all. So in the current 
state I can't use this clang-tidy fix.

Since clang-format doesn't fully parse the code I wonder whether all cases can 
be properly fixed. On the other hand clang-tidy has all information available.

Would integrating clang-format in clang-tidy mean clang-format will use the 
full information of the AST?




Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp:66
+  ConstAlignment(
+  Options.get("ConstAlignment", utils::fixit::QualifierPolicy::Right)) 
{
   if (AnalyzeValues == false && AnalyzeReferences == false)

njames93 wrote:
> Mordante wrote:
> > I would suggest to use `QualifierAlignment` to match the name in 
> > clang-format.
> I thought about that, but the clang-format option doesn't just align the 
> const qualifier. It works for all qualifiers.
> I am easy on what name we use for the option.
Fair point.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst:149
 int *changing_pointee = 
 changing_pointee = 
 

Do you want to incorporate the D131859 here too or do you prefer that I make a 
separate review for that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D131386#3723164 , @aaron.ballman 
wrote:

> In D131386#3723144 , @carlosgalvezp 
> wrote:
>
>>> but removing those options can break people's .clang-tidy config files
>>
>> Aren't there deprecation mechanisms? I think trying to be backwards 
>> compatible across all possible versions can lead to suboptimal solutions and 
>> make the tool harder to use and hard/slow to adapt to the needs of the 
>> community.
>
> Nothing stops us from deprecating config options with some nice diagnostic 
> behavior, but again, this pushes the work off onto the user to maintain their 
> scripts and gives them an inconsistent interface to clang-tidy where some 
> checks get formatting options and other checks do not.

Yes, I'd agree that the interface becomes inconsistent. Best would be a unified 
tool that does both fix and formatting as you mentioned earlier.

FWIW, there are already other checks that have formatting settings, for example:
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-bounds-constant-array-index.html#cmdoption-arg-includestyle


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D131386#3723144 , @carlosgalvezp 
wrote:

>> but removing those options can break people's .clang-tidy config files
>
> Aren't there deprecation mechanisms? I think trying to be backwards 
> compatible across all possible versions can lead to suboptimal solutions and 
> make the tool harder to use and hard/slow to adapt to the needs of the 
> community.

Nothing stops us from deprecating config options with some nice diagnostic 
behavior, but again, this pushes the work off onto the user to maintain their 
scripts and gives them an inconsistent interface to clang-tidy where some 
checks get formatting options and other checks do not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

> but removing those options can break people's .clang-tidy config files

Aren't there deprecation mechanisms? I think trying to be backwards compatible 
across all possible versions can lead to suboptimal solutions and make the tool 
harder to use and hard/slow to adapt to the needs of the community.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D131386#3722849 , @aaron.ballman 
wrote:

> In D131386#3722822 , @njames93 
> wrote:
>
>> In D131386#3722749 , 
>> @aaron.ballman wrote:
>>
>>> We leave formatting decisions in clang-tidy to clang-format and I don't 
>>> think we should deviate from that policy here without a very clear 
>>> understanding of when we should relax that restriction. That said, I'm 
>>> personally not certain we should have such an option (the long-term goal 
>>> has generally been to integrate clang-format functionality into clang-tidy 
>>> so there can be an option to just run format after applying fixes in a TU). 
>>> Is there a compelling reason we should have it?
>>
>> The reason for this is due to the issue that `QualifierAlignment`  is a non 
>> whitespace only change and clang-format lists that using it could break some 
>> code.
>
> Yes, and the community decided that risk was reasonable for clang-format.
>
>> In light of this some users may wish to set the option to `QAS_Leave` to be 
>> sure no code is broken even though they would prefer a specific style. 
>> Therefore having a dedicated option in the check will let those users 
>> specify the style, without having to set a clang-format configuration which 
>> they aren't content in using.
>
> When we decided that clang-format was allowed to break code, we also decided 
> that any time it does so on real world code is considered a bug and is 
> something we should work to fix. So I'm not in favor of this change; if 
> clang-tidy uncovers a bug in clang-format, that should be fixed in 
> clang-format rather than worked around in clang-tidy. And if clang-tidy can't 
> trust clang-format to be production quality, that's something we should 
> address with the clang-format folks as a serious concern that needs a plan of 
> action. But I don't think introducing formatting options into clang-tidy is a 
> road we want to go down.

I suppose another way to look at it is: clang-format options that are 
considered dangerous (comes with a warning as QualifierAlignment does in 
https://clang.llvm.org/docs/ClangFormatStyleOptions.html) are things clang-tidy 
checks with fixes could have options for. However, that gets awkward when 
clang-format finds a way to remove that warning; then clang-tidy is left 
supporting formatting options that it shouldn't have (but removing those 
options can break people's .clang-tidy config files), so I'm not certain I like 
that approach any better.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D131386#3722822 , @njames93 wrote:

> In D131386#3722749 , @aaron.ballman 
> wrote:
>
>> We leave formatting decisions in clang-tidy to clang-format and I don't 
>> think we should deviate from that policy here without a very clear 
>> understanding of when we should relax that restriction. That said, I'm 
>> personally not certain we should have such an option (the long-term goal has 
>> generally been to integrate clang-format functionality into clang-tidy so 
>> there can be an option to just run format after applying fixes in a TU). Is 
>> there a compelling reason we should have it?
>
> The reason for this is due to the issue that `QualifierAlignment`  is a non 
> whitespace only change and clang-format lists that using it could break some 
> code.

Yes, and the community decided that risk was reasonable for clang-format.

> In light of this some users may wish to set the option to `QAS_Leave` to be 
> sure no code is broken even though they would prefer a specific style. 
> Therefore having a dedicated option in the check will let those users specify 
> the style, without having to set a clang-format configuration which they 
> aren't content in using.

When we decided that clang-format was allowed to break code, we also decided 
that any time it does so on real world code is considered a bug and is 
something we should work to fix. So I'm not in favor of this change; if 
clang-tidy uncovers a bug in clang-format, that should be fixed in clang-format 
rather than worked around in clang-tidy. And if clang-tidy can't trust 
clang-format to be production quality, that's something we should address with 
the clang-format folks as a serious concern that needs a plan of action. But I 
don't think introducing formatting options into clang-tidy is a road we want to 
go down.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 452630.
njames93 marked an inline comment as done.
njames93 added a comment.

Remove unneeded include and add Release notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

Files:
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",WEST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: west, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",WEST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: left, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",EAST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: east, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",EAST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: right, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+
+void foo() {
+  int I = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'I' of type 'int' can be declared 'const'
+  // CHECK-FIXES-WEST: const int I = 0;
+  // CHECK-FIXES-EAST: int const I = 0;
+}
+
+void foo(int I) {
+  int & IRef = I;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'IRef' of type 'int &' can be declared 'const'
+  // CHECK-FIXES-WEST: const int & IRef = I;
+  // Having the double space and the const attached the the `&` is unfortunate,
+  // however it shouldn't matter for users as fixes get clang-formatted.
+  // CHECK-FIXES-EAST: int  const& IRef = I;
+
+  // When decorating a pointer with const, the const always goes to the right 
+  // of the `*`.
+  int *IPtr = 
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'IPtr' of type 'int *' can be declared 'const'
+  // CHECK-FIXES: int *const IPtr = 
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
@@ -148,3 +148,25 @@
 int *changing_pointee = 
 changing_pointee = 
 
+.. option:: ConstAlignment 
+
+  Controls where to insert ``const`` when creating fix-it hints, defaults to ``right``.
+
+  - ``left``:
+
+   .. code-block:: c++ 
+
+ const int I = 0;
+ const int  = I;
+ int *const IPtr = 
+
+
+  - ``right``:
+
+   .. code-block:: c++
+  
+ int const I = 0;
+ int const  = I;
+ int *const IPtr = 
+
+  ``east`` and ``west`` aliases are also supported.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -121,6 +121,10 @@
   support for C++14 signal handler rules was added. Bug report generation was
   improved.
 
+- :doc:`misc-const-correctness `
+  check now supports a ``ConstAlignment`` option to control the location of the
+  ``const`` keyword when emitting fixes.
+
 Removed checks
 ^^
 
Index: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
===
--- clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
+++ clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONSTCORRECTNESSCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "../utils/FixItHintUtils.h"
 #include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "llvm/ADT/DenseSet.h"
 
@@ -48,6 +49,8 @@
   const bool TransformValues;
   const bool TransformReferences;
   const bool TransformPointersAsValues;
+
+  

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Nathan James via Phabricator via cfe-commits]

njames93 added a reviewer: Mordante.
njames93 added a comment.

In D131386#3722749 , @aaron.ballman 
wrote:

> We leave formatting decisions in clang-tidy to clang-format and I don't think 
> we should deviate from that policy here without a very clear understanding of 
> when we should relax that restriction. That said, I'm personally not certain 
> we should have such an option (the long-term goal has generally been to 
> integrate clang-format functionality into clang-tidy so there can be an 
> option to just run format after applying fixes in a TU). Is there a 
> compelling reason we should have it?

The reason for this is due to the issue that `QualifierAlignment`  is a non 
whitespace only change and clang-format lists that using it could break some 
code.
In light of this some users may wish to set the option to `QAS_Leave` to be 
sure no code is broken even though they would prefer a specific style. 
Therefore having a dedicated option in the check will let those users specify 
the style, without having to set a clang-format configuration which they aren't 
content in using.




Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp:66
+  ConstAlignment(
+  Options.get("ConstAlignment", utils::fixit::QualifierPolicy::Right)) 
{
   if (AnalyzeValues == false && AnalyzeReferences == false)

Mordante wrote:
> I would suggest to use `QualifierAlignment` to match the name in clang-format.
I thought about that, but the clang-format option doesn't just align the const 
qualifier. It works for all qualifiers.
I am easy on what name we use for the option.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

We leave formatting decisions in clang-tidy to clang-format and I don't think 
we should deviate from that policy here without a very clear understanding of 
when we should relax that restriction. That said, I'm personally not certain we 
should have such an option (the long-term goal has generally been to integrate 
clang-format functionality into clang-tidy so there can be an option to just 
run format after applying fixes in a TU). Is there a compelling reason we 
should have it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Mark de Wever via Phabricator via cfe-commits]

Mordante added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp:10
 #include "ConstCorrectnessCheck.h"
 #include "../utils/FixItHintUtils.h"
 #include "clang/AST/ASTContext.h"

This is already included in the header.



Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp:66
+  ConstAlignment(
+  Options.get("ConstAlignment", utils::fixit::QualifierPolicy::Right)) 
{
   if (AnalyzeValues == false && AnalyzeReferences == false)

I would suggest to use `QualifierAlignment` to match the name in clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: aaron.ballman, JonasToth.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
njames93 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Adds an option to specify where the `const` should be placed when emitting 
fixes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131386

Files:
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",WEST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: west, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",WEST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: left, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",EAST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: east, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",EAST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: right, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+
+void foo() {
+  int I = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'I' of type 'int' can be declared 'const'
+  // CHECK-FIXES-WEST: const int I = 0;
+  // CHECK-FIXES-EAST: int const I = 0;
+}
+
+void foo(int I) {
+  int & IRef = I;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'IRef' of type 'int &' can be declared 'const'
+  // CHECK-FIXES-WEST: const int & IRef = I;
+  // Having the double space and the const attached the the `&` is unfortunate,
+  // however it shouldn't matter for users as fixes get clang-formatted.
+  // CHECK-FIXES-EAST: int  const& IRef = I;
+
+  // When decorating a pointer with const, the const always goes to the right 
+  // of the `*`.
+  int *IPtr = 
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'IPtr' of type 'int *' can be declared 'const'
+  // CHECK-FIXES: int *const IPtr = 
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
@@ -148,3 +148,25 @@
 int *changing_pointee = 
 changing_pointee = 
 
+.. option:: ConstAlignment 
+
+  Controls where to insert ``const`` when creating fix-it hints, defaults to ``right``.
+
+  - ``left``:
+
+   .. code-block:: c++ 
+
+ const int I = 0;
+ const int  = I;
+ int *const IPtr = 
+
+
+  - ``right``:
+
+   .. code-block:: c++
+  
+ int const I = 0;
+ int const  = I;
+ int *const IPtr = 
+
+  ``east`` and ``west`` aliases are also supported.
Index: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
===
--- clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
+++ clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONSTCORRECTNESSCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "../utils/FixItHintUtils.h"
 #include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "llvm/ADT/DenseSet.h"
 
@@ -48,6 +49,8 @@
   const bool TransformValues;
   const bool TransformReferences;
   const bool TransformPointersAsValues;
+
+  const utils::fixit::QualifierPolicy ConstAlignment;
 };
 
 } // namespace misc
Index: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
@@ -18,6 +18,22 @@
 
 namespace clang {
 namespace tidy {
+