Page MenuHomePhabricator

[clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness
Needs ReviewPublic

Authored by JonasToth on Nov 27 2018, 4:04 AM.

Details

Summary

This patch connects the check for const-correctness with the new general
utility to add const to variables.
The code-transformation is only done, if the detected variable for const-ness
is not part of a group-declaration.

This patch (in combination with readability-isolate-declaration) shows some
false positives of the ExprMutAnalyzer that should be addressed, as they
result in wrong code-transformation.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • accidentally wrong patch uploaded
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2019, 1:38 AM
Herald added subscribers: mgehre, wuzish. · View Herald Transcript
JonasToth updated this revision to Diff 230048.Nov 19 2019, 5:12 AM
  • port patch to monorepo
  • Merge branch 'master' into feature_transform_const.patch
  • merge current master and new version for previous steps into this patch
  • fix compilation issues after type renaming
  • work on fixing support and find out issues
JonasToth updated this revision to Diff 235712.Tue, Dec 31, 7:06 AM
  • fix error from macro-defined variables
  • move test files in sub-directory
  • fix false positive of news in type-dependent contextes
  • add a true positive in typedependent context
  • update to latest utility version

shows a couple of false positives.

JonasToth updated this revision to Diff 236101.Fri, Jan 3, 11:56 AM
  • ignore implicit casts in memberExpr(hasObjectExpression())
  • implement functionality for function pointers

shows a couple of false positives.

I added your tests, but some did not show false positives anymore. I think i will remove some, that i find redundant.
But could you please recheck with the current version first, if this is actually correct and the original false positives are gone?

JonasToth marked 2 inline comments as done.Fri, Jan 3, 11:59 AM
JonasToth added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
5265

I will remove those later. They are not used.
Tidying up will be done, once i dont find any false positives/false transformations anymore.

clang/lib/Analysis/ExprMutationAnalyzer.cpp
30–32

these are formating changes, i will revert them / apply them as separate commit.

shows a couple of false positives.

I added your tests, but some did not show false positives anymore. I think i will remove some, that i find redundant.
But could you please recheck with the current version first, if this is actually correct and the original false positives are gone?

I can confirm that the false positives in the real code have been cleaned up. Thank you!

However I would be loath to discard tests - unless we can prove they are truly redundant.

Also, here is a new test for you - it trips now on "unsigned someValue = 0;" line:

struct IntWrapper {
   unsigned low;
   unsigned high;

   IntWrapper& operator=(unsigned value) {
      low = value & 0xffff;
      high = (value >> 16) & 0xffff;
   }

   template<typename Istream>
   friend Istream& operator>>(Istream& is, IntWrapper& rhs) {
      unsigned someValue = 0;
      if (is >> someValue) {
         rhs = someValue;
      }
      return is;
   }
};


unsigned TestHiddenFriend(IntMaker& im) {
   IntWrapper iw;

   im >> iw;

   return iw.low;
}

Indicated where the new test code should go.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

Please insert the this test code here:

struct IntWrapper {

   unsigned low;
   unsigned high;

   IntWrapper& operator=(unsigned value) {
      low = value & 0xffff;
      high = (value >> 16) & 0xffff;
   }

   template<typename Istream>
   friend Istream& operator>>(Istream& is, IntWrapper& rhs) {
      unsigned someValue = 0;       // false positive now
      if (is >> someValue) {
         rhs = someValue;
      }
      return is;
   }
};

unsigned TestHiddenFriend(IntMaker& im) {
   IntWrapper iw;

   im >> iw;

   return iw.low;
}
JonasToth marked 2 inline comments as done.Sat, Jan 4, 7:24 AM
JonasToth added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

clang gives me no error when I add the const there. sure it does reproduce properly?

JonasToth marked 2 inline comments as done.Sat, Jan 4, 7:30 AM
JonasToth added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

Here for reference: https://godbolt.org/z/DXKMYh

0x8000-0000 added inline comments.Sat, Jan 4, 7:31 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

Probably I wasn't clear - I suggested adding my test code at line 608, because it needs the "IntMaker" definition at line 594.

The false positive from this const check is reported on the "unsigned someValue = 0;" line inside the template extraction operator.

0x8000-0000 added inline comments.Sat, Jan 4, 7:41 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

Oh, I got it - don't think "shift" think "overloaded extraction operator".

In my code above, you don't know what ">>" does, and it clearly takes a mutable reference as the right side.

const int foo;
std::cin >> foo;

should not compile, either :)

This patch does not build on top of current tree:

/usr/bin/ld: lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in function `clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65: undefined reference to `clang::ast_matchers::decompositionDecl'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[2771/4997] Building CXX object tools/clang/tools/extra/clang-include-fixer/find-all-symbols/CMakeFiles/obj.findAllSymbols.dir/PragmaCommentHandler.cpp.o
ninja: build stopped: subcommand failed.

This is my top of the tree right now:

b7ecf1c1c373c53183ef6ef66efbe4237ff7b96d (origin/master, origin/HEAD, master) NFC: Fix trivial typos in comments

This patch does not build on top of current tree:

/usr/bin/ld: lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in function `clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65: undefined reference to `clang::ast_matchers::decompositionDecl'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[2771/4997] Building CXX object tools/clang/tools/extra/clang-include-fixer/find-all-symbols/CMakeFiles/obj.findAllSymbols.dir/PragmaCommentHandler.cpp.o
ninja: build stopped: subcommand failed.

This is my top of the tree right now:

b7ecf1c1c373c53183ef6ef66efbe4237ff7b96d (origin/master, origin/HEAD, master) NFC: Fix trivial typos in comments

did the changes to clang/include/clang/ASTMatchers/ASTMatchers.h and clang/lib/ASTMatchers/Dynamic/Registry.cpp apply? They provide the decompositionDecl matcher. It seems odd, they are not available :/

JonasToth marked an inline comment as done.Sat, Jan 4, 10:47 AM
JonasToth added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

no. something else is going on.
https://godbolt.org/z/xm8eVC

| |   |-CXXOperatorCallExpr <line:21:5, col:11> '<dependent type>'
| |   | |-UnresolvedLookupExpr <col:8> '<overloaded function type>' lvalue (ADL) = 'operator>>' 0x55a26b885938 0x55a26b857238
| |   | |-DeclRefExpr <col:5> 'Istream' lvalue ParmVar 0x55a26b885748 'is' 'Istream &'
| |   | `-DeclRefExpr <col:11> 'const unsigned int' lvalue Var 0x55a26b885a38 'someValue' 'const unsigned int'

This code is only a false positive in the sense, that the "we can not know if something bad happens" is not detected. The operator>> is not resolved. That is because the template is not instantiated and the expressions can therefore not be resolved yet.
There seems to be no instantiation of this template function.

Somehow the im >> iw; does not instantiate the friend operator<<. I reduced it to something i think is minimal and that shows the same behaviour. (https://godbolt.org/z/MMG_4q)

This patch does not build on top of current tree:

/usr/bin/ld: lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in function `clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65: undefined reference to `clang::ast_matchers::decompositionDecl'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[2771/4997] Building CXX object tools/clang/tools/extra/clang-include-fixer/find-all-symbols/CMakeFiles/obj.findAllSymbols.dir/PragmaCommentHandler.cpp.o
ninja: build stopped: subcommand failed.

This is my top of the tree right now:

b7ecf1c1c373c53183ef6ef66efbe4237ff7b96d (origin/master, origin/HEAD, master) NFC: Fix trivial typos in comments

did the changes to clang/include/clang/ASTMatchers/ASTMatchers.h and clang/lib/ASTMatchers/Dynamic/Registry.cpp apply? They provide the decompositionDecl matcher. It seems odd, they are not available :/

This is the result of applying the patch as e-mailed:

~/tools/llvm-project$ patch -p0 < /tmp/D54943.236101.patch                                                                                                                    
patching file clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp                                                                                                           
patching file clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp                                                                                                        
patching file clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp                                                                                                        
patching file clang/lib/Analysis/ExprMutationAnalyzer.cpp                                                                                                                     
patching file clang/lib/ASTMatchers/Dynamic/Registry.cpp                                                                                                                      
patching file clang/include/clang/ASTMatchers/ASTMatchers.h                                                                                                                   
patching file clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp                                                                       
patching file clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp                                                             
patching file clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp                                                  
patching file clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp                                                            
patching file clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp                                                                        
patching file clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst                                                                                
patching file clang-tools-extra/docs/ReleaseNotes.rst                                                                                                                         
patching file clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp                                                                                  
patching file clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h                                                                                          
patching file clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp                                                                                        
patching file clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt                                                                                                   
patching file clang-tools-extra/CMakeLists.txt

No rejection that I can see.

~/tools/llvm-project$ ag decompositionDecl                                    
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
67:                                 unless(has(decompositionDecl()))))

clang/include/clang/ASTMatchers/ASTMatchers.h
318:extern const internal::VariadicAllOfMatcher<DecompositionDecl> decompositionDecl;

clang/lib/ASTMatchers/Dynamic/Registry.cpp
197:  REGISTER_MATCHER(decompositionDecl);
0x8000-0000 added inline comments.Sat, Jan 4, 12:38 PM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

https://godbolt.org/z/7QEdHJ

struct IntMaker {
  operator bool() const;

  friend IntMaker &operator>>(IntMaker &, unsigned &);
  //friend IntMaker &operator>>(IntMaker &, const unsigned &) = delete;
};

If you uncomment the deleted overload then

template <typename Istream>
Istream& operator>>(Istream& is, IntWrapper& rhs)  {
    unsigned const someValue = 0;
    if (is >> someValue) {
        rhs = someValue;
    }
    return is;
}

Fails to compile.

Depending on what else is around, it seems that somehow the compiler would try to use the (bool) conversion to obtain an integral then use it as an argument to the built-in integral left shift.

https://godbolt.org/z/-JFL5o - this does not compile, as expected:

#include <iostream>

int readInt() {
    const int foo = 0;
    std::cin >> foo;
    return foo;
}

so this check should not recommend making foo constant.

JonasToth marked an inline comment as done.Sat, Jan 4, 2:25 PM
JonasToth added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

I see. Implicit conversions are great... :)

I will recheck that. And the std::cin example is analyzed correctly. I added a test for that, too.
Thank you for researching the issue!

JonasToth marked 4 inline comments as done.Sun, Jan 5, 5:07 AM
JonasToth added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

https://godbolt.org/z/VerWce
Minimal example that shows the issue.

0x8000-0000 marked an inline comment as done.Sun, Jan 5, 7:57 AM
0x8000-0000 added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
609

As much as I would have liked to contribute a good test to your new checker, I'm happier that I have found something strange with my original code from which I was trying to distill the minimal case.

For what it's worth, dumping the AST on my original code, shows this

|     | |       |-IfStmt 0x7fdafcfb3a60 <line:60:9, line:63:9>
|     | |       | |-BinaryOperator 0x7fdafcfb32b0 <line:60:13, col:19> '<dependent type>' '>>'
|     | |       | | |-DeclRefExpr 0x7fdafcfb3270 <col:13> 'Istream' lvalue ParmVar 0x7fdafcfb1448 'is' 'Istream &'
|     | |       | | `-DeclRefExpr 0x7fdafcfb3290 <col:19> 'uint32_t':'unsigned int' lvalue Var 0x7fdafcfb31b8 'someValue' 'uint32_t':'unsigned int'

Instead of the expected:

2220 | |   |   |-IfStmt 0x5971b00 <line:624:7, line:626:7>                                                                                                   
2221 | |   |   | |-CXXOperatorCallExpr 0x596f040 <line:624:11, col:17> '<dependent type>'
2222 | |   |   | | |-UnresolvedLookupExpr 0x596efe8 <col:14> '<overloaded function type>' lvalue (ADL) = 'operator>>' 0x596b8a8 0x596b668 0x596ab38          
2223 | |   |   | | |-DeclRefExpr 0x596efa8 <col:11> 'Istream' lvalue ParmVar 0x596e968 'is' 'Istream &'                                                      
2224 | |   |   | | `-DeclRefExpr 0x596efc8 <col:17> 'unsigned int' lvalue Var 0x596eef0 'someValue' 'unsigned int'

I need to investigate if I need to shuffle some includes around to get clang to treat it as an overloaded method instead of shift. At any rate, the new const-checker has proved its worth by pointing me to a bug!

Thank you, Jonas!

JonasToth updated this revision to Diff 236253.Sun, Jan 5, 10:02 AM
  • find more false positives
  • fix false positive with refernces to containers in range-for. This introduces false negative though
  • new test-case for check
  • fix range-for iterator speciality; start working on complex conditional and comma-operator expression that result in a mutation on a variable
  • ignore implicit casts for member-call expr
  • fix another false positive in typeDependent contexts and memberExpr instead of cxxMemberCallExpr
  • add test for overloaded operator>>
  • add test for templated operator>>
  • another case fixed
  • add test for member access in for loop init
  • fix false positive with member access with implicit cast between sub and base-class
  • address explicit cast to references of an element
JonasToth added a comment.EditedSun, Jan 5, 10:07 AM

clang/lib almost transforms cleanly.
issues still left:

  • InitListExpr can initialize non-const references. Such cases are not figured out yet (see create_false_positive at the end of the testcases)
  • I found a case in clang, where const was correctly added to a variable, but that variable was used in a conditional operator (foo ? const_var : non_const_var) which broke compilation. I will not fix this, as this is extremly rare (I suppose) and in a sense defeats the purpose of our analysis. I think something like this should be fixed by hand with a NOLINT.

I continue to investigate false positives. But I think most of the real world problems are somewhat figured out now and i will start bringing the patch into better shape and might even split it up in ExprMutAnalyzer part and clang-tidy part.

Edit: Minimal example for the InitListExpr-issue: https://godbolt.org/z/Co-RDN

I can't build this on top of current llvm-project tree (6a6e6f04ec2cd2f4f07ec4943036c5c2d47ce0c7):

/usr/bin/ld: lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in function `clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65: undefined reference to `clang::ast_matchers::decompositionDecl'

I am building using clang++-9 from Debian, and diff7 applied cleanly.

Jonas, is it possible to rebase this on top of llvm-project and push it to a branch on https://github.com/JonasToth/llvm-project ?

I can't build this on top of current llvm-project tree (6a6e6f04ec2cd2f4f07ec4943036c5c2d47ce0c7):

/usr/bin/ld: lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in function `clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65: undefined reference to `clang::ast_matchers::decompositionDecl'

I am building using clang++-9 from Debian, and diff7 applied cleanly.

Jonas, is it possible to rebase this on top of llvm-project and push it to a branch on https://github.com/JonasToth/llvm-project ?

I just merged master into the branch and pushed to feature_transform_const.patch branch in my fork. Maybe that works?
If you want you can join IRC, its faster to get this fixed there :)

I can't build this on top of current llvm-project tree (6a6e6f04ec2cd2f4f07ec4943036c5c2d47ce0c7):

/usr/bin/ld: lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in function `clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65: undefined reference to `clang::ast_matchers::decompositionDecl'

I am building using clang++-9 from Debian, and diff7 applied cleanly.

Jonas, is it possible to rebase this on top of llvm-project and push it to a branch on https://github.com/JonasToth/llvm-project ?

I just merged master into the branch and pushed to feature_transform_const.patch branch in my fork. Maybe that works?
If you want you can join IRC, its faster to get this fixed there :)

Strange - I have checked out your branch, and the patch is correct (no difference between when I apply the phabricator diff7 on top of upstream llvm-project and when i look at feature_transform_const.patch)

I am using the following cmake configuration:

cmake ../llvm \
   -DCMAKE_INSTALL_PREFIX=/opt/clang10 \
   -DCMAKE_C_COMPILER=/usr/bin/clang-9 \
   -DCMAKE_CXX_COMPILER=/usr/bin/clang++-9 \
   -DCMAKE_BUILD_TYPE=Debug \
   -DLLVM_TARGETS_TO_BUILD=X86 \
   -DLLVM_ENABLE_THREADS=On \
   -DLLVM_INSTALL_TOOLCHAIN_ONLY=On \
   -DLLVM_USE_SPLIT_DWARF=On \
   -DLLVM_BUILD_TESTS=ON \
   -DLLVM_ENABLE_PROJECTS="clang;libcxx;libcxxabi;libunwind;lldb;compiler-rt;lld;clang-tools-extra;" \
   -GNinja

What cmake configuration are you using?

(Sorry - I need to go, no time for IRC today)

I can't build this on top of current llvm-project tree (6a6e6f04ec2cd2f4f07ec4943036c5c2d47ce0c7):

/usr/bin/ld: lib/libclangTidyCppCoreGuidelinesModule.a(ConstCorrectnessCheck.cpp.o): in function `clang::tidy::cppcoreguidelines::ConstCorrectnessCheck::registerMatchers(clang::ast_matchers::MatchFinder*)':
/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:65: undefined reference to `clang::ast_matchers::decompositionDecl'

I am building using clang++-9 from Debian, and diff7 applied cleanly.

Jonas, is it possible to rebase this on top of llvm-project and push it to a branch on https://github.com/JonasToth/llvm-project ?

I just merged master into the branch and pushed to feature_transform_const.patch branch in my fork. Maybe that works?
If you want you can join IRC, its faster to get this fixed there :)

Strange - I have checked out your branch, and the patch is correct (no difference between when I apply the phabricator diff7 on top of upstream llvm-project and when i look at feature_transform_const.patch)

I am using the following cmake configuration:

cmake ../llvm \
   -DCMAKE_INSTALL_PREFIX=/opt/clang10 \
   -DCMAKE_C_COMPILER=/usr/bin/clang-9 \
   -DCMAKE_CXX_COMPILER=/usr/bin/clang++-9 \
   -DCMAKE_BUILD_TYPE=Debug \
   -DLLVM_TARGETS_TO_BUILD=X86 \
   -DLLVM_ENABLE_THREADS=On \
   -DLLVM_INSTALL_TOOLCHAIN_ONLY=On \
   -DLLVM_USE_SPLIT_DWARF=On \
   -DLLVM_BUILD_TESTS=ON \
   -DLLVM_ENABLE_PROJECTS="clang;libcxx;libcxxabi;libunwind;lldb;compiler-rt;lld;clang-tools-extra;" \
   -GNinja

What cmake configuration are you using?

(Sorry - I need to go, no time for IRC today)

soooo weird. it builds both on my laptop and my workstation with comparable configuration (not debug, but RelWithDebInfo). i do use lld and ninja, not sure if that makes any difference (it shouldnt)
i have no idea why it does not link. are those builds clean?

JonasToth updated this revision to Diff 236259.Sun, Jan 5, 12:16 PM
  • Merge branch 'master' into feature_transform_const.patch
  • link clangAnalysis to the cppcoreguidelines-module
  • fix InitListExpr as well
  • Merge branch 'master' into feature_transform_const.patch
  • link clangAnalysis to the cppcoreguidelines-module
  • fix InitListExpr as well

This last version too builds with RelWithDebInfo and fails with Debug configuration (both clean builds).

  • Merge branch 'master' into feature_transform_const.patch
  • link clangAnalysis to the cppcoreguidelines-module
  • fix InitListExpr as well

This last version too builds with RelWithDebInfo and fails with Debug configuration (both clean builds).

So it only fails in debug-build?

JonasToth updated this revision to Diff 236339.Mon, Jan 6, 5:54 AM
  • fix false positive with parenListExpr, dont match so narrow for them
  • try removing more false positives, does not work

This last version too builds with RelWithDebInfo and fails with Debug configuration (both clean builds).

So it only fails in debug-build?

Correct; I can build 'Release' or 'RelWithDebInfo' but not 'Debug'.

JonasToth marked an inline comment as done.Wed, Jan 8, 2:41 PM
JonasToth added inline comments.
clang/lib/Analysis/ExprMutationAnalyzer.cpp
20
JonasToth added a comment.EditedWed, Jan 8, 4:08 PM

I did evaluate the performance of the check on LLVM with the script in the attachement.

The result, after splitting all multi-decl statements with readability-isolate-declarations (which produces no issues) the current analysis for const is run for values, references and pointers (the pointer itself, not the pointee) and the automated fixes are applied. This is done for each TU in LLVM/{llvm,clang,clang-tools-exta,lld}, including unit-tests. Simply each target that is in the compilation_commands.json.

The patch for that:


That is 3697 files changed, 164'596 insertions(+), 164'596 deletions(-)

This patch unfortunatly does not compile cleanly anymore. To fix LLVM up I had to manually fix the following things:


These are 34 places that create compilation failures, not all of them are to blame on the analysis/transformation.

The only actual deficits in analysis seem to be:

  • overloaded operators for templated classes that are unresolved
  • calls to methods that are called through captured this im lambda expression within templated function

These are ~4 instances in all of LLVM.

  • MyType *& my_var = foo(); *(my_var + 3) = 42; the pointer would be qualified const, because the pointee-analysis does not exist. By default pointers will not be marked as const. References to pointers should probably be excluded as well.

Other problems are:

  • the analysis looks "through" reference to a local, as long as they are local. something like int foo;int & my_ref = static_cast<int &>(foo); for each variable not being modified will result in compile errors, becase static_cast<int &>() is not transformed to static_cast<int const&>() -> I have the code to remove such occasions and treat the cast as modification, on the other hand you don't get the gain from adding const.
  • the class does not provide a userdefined default constructor, after applying const there is a MyType const my_var -->{}<--; missing -> can be added with clang-tidy as well
  • ternary operator has a const type for true, and non-const for false
  • MyType const my_var; decltype(my_var); stops working
  • in one test a type-trait that included a decltype stopped working, probably same problem as previous point

I do not consider the latter issues to actually be an issue in the analysis/transformation. They are very seldom and for such cases NOLINT is ok in my opinion.

This leads to LLVM compiling cleanly.

$ ninja check-all
>
> ********************

Testing Time: 442.99s
********************
Failing Tests (9):
    LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestBasicAliases
    LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestBasicReExports
    LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestChainedAliases
    LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestReexportsGenerator
    LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestThatReExportsDontUnnecessarilyMaterialize
    LLVM :: ExecutionEngine/OrcLazy/common-symbols.ll
    LLVM :: ExecutionEngine/OrcLazy/global-ctors-and-dtors.ll
    LLVM :: ExecutionEngine/OrcLazy/global_aliases.ll
    LLVM :: ExecutionEngine/OrcLazy/hidden-visibility.ll

  Expected Passes    : 54838
  Expected Failures  : 175
  Unsupported Tests  : 531
  Unexpected Failures: 9
FAILED: CMakeFiles/check-all

:(
There is an assertion-failure for Orc-stuff.

--
lli: /data/big/llvm-project/llvm/include/llvm/ExecutionEngine/Orc/SymbolStringPool.h:146: llvm::orc::SymbolStringPool::~SymbolStringPool(): Assertion `Pool.empty() && "Dangling references at pool destruction time"' failed.
Stack dump:
0.      Program arguments: /data/big/test-build/bin/lli -jit-kind=orc-lazy -extra-module /data/big/llvm-project/llvm/test/ExecutionEngine/OrcLazy/Inputs/hidden-definitions.ll /data/big/llvm-pr
oject/llvm/test/ExecutionEngine/OrcLazy/hidden-visibility.ll
 #0 0x00007f8626fbbd4f llvm::sys::PrintStackTrace(llvm::raw_ostream&) /data/big/llvm-project/llvm/lib/Support/Unix/Signals.inc:548:13
 #1 0x00007f8626fb9fd0 llvm::sys::RunSignalHandlers() /data/big/llvm-project/llvm/lib/Support/Signals.cpp:69:18
 #2 0x00007f8626fbc155 SignalHandler(int) /data/big/llvm-project/llvm/lib/Support/Unix/Signals.inc:380:3
 #3 0x00007f86282ee770 __restore_rt (/lib64/libpthread.so.0+0x15770)
 #4 0x00007f86268cf3c1 raise (/lib64/libc.so.6+0x3a3c1)
 #5 0x00007f86268b755b abort (/lib64/libc.so.6+0x2255b)
 #6 0x00007f86268b742f (/lib64/libc.so.6+0x2242f)
 #7 0x00007f86268c6a92 (/lib64/libc.so.6+0x31a92)
 #8 0x00007f8628e34f72 std::mutex::lock() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/std_mutex.h:104:2
 #9 0x00007f8628e34f72 std::lock_guard<std::mutex>::lock_guard(std::mutex&) /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/std_mutex.h:159:19
#10 0x00007f8628e34f72 llvm::orc::SymbolStringPool::clearDeadEntries() /data/big/llvm-project/llvm/include/llvm/ExecutionEngine/Orc/SymbolStringPool.h:159:31
#11 0x00007f8628e34f72 llvm::orc::SymbolStringPool::~SymbolStringPool() /data/big/llvm-project/llvm/include/llvm/ExecutionEngine/Orc/SymbolStringPool.h:145:3
#12 0x00007f8628e34f72 void __gnu_cxx::new_allocator<llvm::orc::SymbolStringPool>::destroy<llvm::orc::SymbolStringPool>(llvm::orc::SymbolStringPool*) /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/inc
lude/g++-v9/ext/new_allocator.h:153:10
#13 0x00007f8628e34f72 void std::allocator_traits<std::allocator<llvm::orc::SymbolStringPool> >::destroy<llvm::orc::SymbolStringPool>(std::allocator<llvm::orc::SymbolStringPool>&, llvm::orc::S
ymbolStringPool*) /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/alloc_traits.h:497:8
#14 0x00007f8628e34f72 std::_Sp_counted_ptr_inplace<llvm::orc::SymbolStringPool, std::allocator<llvm::orc::SymbolStringPool>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() /usr/lib/gcc/x86_64-pc-l
inux-gnu/9.2.0/include/g++-v9/bits/shared_ptr_base.h:557:2
#15 0x00007f8628e587eb __gnu_cxx::__exchange_and_add_dispatch(int*, int) /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/ext/atomicity.h:81:9
#16 0x00007f8628e587eb std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/shared_ptr_base.h:167:10
#17 0x00007f8628e587eb std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/shared_ptr_base.h:730:11
#18 0x00007f8628e587eb std::__shared_ptr<llvm::orc::SymbolStringPool, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/shared_ptr_base.h:
1169:31
#19 0x00007f8628e587eb llvm::orc::ExecutionSession::~ExecutionSession() /data/big/llvm-project/llvm/include/llvm/ExecutionEngine/Orc/Core.h:1053:7
#20 0x00007f8628e54f96 std::default_delete<llvm::orc::ExecutionSession>::operator()(llvm::orc::ExecutionSession*) const /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/unique_ptr.h:
81:2
#21 0x00007f8628e54f96 std::unique_ptr<llvm::orc::ExecutionSession, std::default_delete<llvm::orc::ExecutionSession> >::~unique_ptr() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits
/unique_ptr.h:284:4
#22 0x00007f8628e54f96 llvm::orc::LLJIT::~LLJIT() /data/big/llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:55:1
#23 0x000000000041c2f1 std::default_delete<llvm::orc::LLLazyJIT>::operator()(llvm::orc::LLLazyJIT*) const /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/unique_ptr.h:81:2
#24 0x000000000041c2f1 std::unique_ptr<llvm::orc::LLLazyJIT, std::default_delete<llvm::orc::LLLazyJIT> >::~unique_ptr() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/unique_ptr.h:

But all failures seem to be this assertion.

Given this result (i mostly consider it a succes, even though the overloaded operators not working confuse me) I want to start cleaning up this patch and start committing e.g. formatting changes and everything that can be extracted from this patch.
What is currently missing are isolated unit-tests in unittests/Analysis for each new fixed code construct. They all live in the clang-tidy test which is suboptimal.
Once the cleaning is successfull I want to address this.

@aaron.ballman and others: what do you think of the quality of the analysis? Should this check be comittable functionality-wise?
IMHO it would really help to get some stuff into master

JonasToth updated this revision to Diff 237136.Thu, Jan 9, 11:23 AM
  • Merge branch 'master' into feature_transform_const.patch
  • actually dismiss function references
  • work on exclusion of variables that have type, autotype or reference-type to template parameter
  • by default analyze the pointers and transform as well, for testing
  • fix canResolveToExpr to account for derivedToBaseClass casts
  • restore unit tests for expr mut analyzer - there was an oversight in my build that lead them not being running
  • short id for matcher
  • fix test for fixed false negative
  • rip out unused matchers that are now unnecessary
  • try to ignore auto-types that are type-dependent
  • consider 'static_cast<Type&>()' always a mutation, speculativly fix tests for that
JonasToth updated this revision to Diff 237317.Fri, Jan 10, 7:15 AM
  • fix unit tests for exprmutanalyzer, whitespace was the problem
  • Merge branch 'master' into feature_transform_const.patch
JonasToth updated this revision to Diff 237345.Fri, Jan 10, 8:46 AM
  • test if references to pointers are ignored if pointers are ignored as values
  • add different possible auto constellations
  • document the problematic cases for 'auto' deductions

I think a first pass of review might make sense now. I isolated the problematic
'auto' cases and added tests for them in clang-tidy. It turns out, that they
are only bad if they deduce to a reference to a template-type/typedef to a
template type.
In good faith, that this is somehow detectable in code, i think all known
false positives can be addressed.
Even though I hope this to land until the branch point, I don't want to pressure
reviewers or so (i chatted with aaron about it)! If the patch is not good enough
or has any outstanding issues, I will of course address them and land it later. :)

0x8000-0000 added a comment.EditedFri, Jan 10, 2:46 PM

Applied the fix-it on one of our smaller (but C++ code bases) and it builds and passes the tests. Comments:

  1. I'm still seeing some false positives, where some locals that are const are flagged as could be made const - I'm working to create a minimal reproduction recipe.
  2. We're a west const shop :) so adding a bunch of east-consts will not fly well. Is there a way to configure where 'const' is placed by the fixit? (Specifically 'auto const', we prefer it 'const auto').

Applied the fix-it on one of our smaller (but C++ code bases) and it builds and passes the tests. Comments:

  1. I'm still seeing some false positives, where some locals that are const are flagged as could be made const - I'm working to create a minimal reproduction recipe.
  2. We're a west const shop :) so adding a bunch of east-consts will not fly well. Is there a way to configure where 'const' is placed by the fixit? (Specifically 'auto const', we prefer it 'const auto').

Does it build now? I couldn't find a way to reproduce that and gave up, tbh.

  1. Template context? Auto involved? I saw some double-analysis for auto&, because clang-tidy didn't ignore those properly. And are you using run-clang-tidy? It deduplicates fixits, maybe that is involved?
  2. YesNo, The utility for adding const is able to do both, west const has problems with typedef int * MyType; scenarios, where the const will apply to the wrong thing. Doing that right requires special care. BUT: clang-format has a east-const/west-const feature now (i think new with this release).

So I am now somewhat considering to let clang-format do that for me.

Thanks again for taking a look at it. But if the issues you found are new, i think we should maybe not commit this weekend.

0x8000-0000 added a comment.EditedFri, Jan 10, 3:56 PM

Applied the fix-it on one of our smaller (but C++ code bases) and it builds and passes the tests. Comments:

  1. I'm still seeing some false positives, where some locals that are const are flagged as could be made const - I'm working to create a minimal reproduction recipe.
  2. We're a west const shop :) so adding a bunch of east-consts will not fly well. Is there a way to configure where 'const' is placed by the fixit? (Specifically 'auto const', we prefer it 'const auto').

Does it build now? I couldn't find a way to reproduce that and gave up, tbh.

  1. Template context? Auto involved? I saw some double-analysis for auto&, because clang-tidy didn't ignore those properly. And are you using run-clang-tidy? It deduplicates fixits, maybe that is involved?
  2. YesNo, The utility for adding const is able to do both, west const has problems with typedef int * MyType; scenarios, where the const will apply to the wrong thing. Doing that right requires special care. BUT: clang-format has a east-const/west-const feature now (i think new with this release). So I am now somewhat considering to let clang-format do that for me.

    Thanks again for taking a look at it. But if the issues you found are new, i think we should maybe not commit this weekend.

I haven't tried Debug build, Release only.

I'm good with having clang-format do the west-const transformation.

It's not a template, but something like "const Foo foo{x, y, z};" where x and y were references and z was a pointer. I'll try to reduce a test case. If it helps in any way, the fix-it was trying to change the code to "const Foo const foo{x, y, z}".

I have also noticed a check failure, but not sure if in this tool or not.

Here is a minimal example that shows the problem:

#include <memory>

template<typename SomeValue>
struct DoGooder
{
    DoGooder(void* accessor, SomeValue value)
    {
    }

};

struct Bingus
{
    static constexpr auto someRandomConstant = 99;
};

template<typename Foo>
struct HardWorker
{
    HardWorker()
    {
        const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
    }
};

struct TheContainer
{
    std::unique_ptr<HardWorker<Bingus>> m_theOtherInstance;
};

Example run:

$ /opt/clang10/bin/clang-tidy -checks="-*,cppcoreguidelines-const-correctness" -header-filter=".*" reproconst.cpp -- -std=c++17 -Wall
54 warnings generated.
reproconst.cpp:22:9: warning: variable 'anInstanceOf' of type '<dependent type>' can be declared 'const' [cppcoreguidelines-const-correctness]
        const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
        ^
                       const
Suppressed 53 warnings (53 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

Here is a minimal example that shows the problem:

#include <memory>

template<typename SomeValue>
struct DoGooder
{
    DoGooder(void* accessor, SomeValue value)
    {
    }

};

struct Bingus
{
    static constexpr auto someRandomConstant = 99;
};

template<typename Foo>
struct HardWorker
{
    HardWorker()
    {
        const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
    }
};

struct TheContainer
{
    std::unique_ptr<HardWorker<Bingus>> m_theOtherInstance;
};

Example run:

$ /opt/clang10/bin/clang-tidy -checks="-*,cppcoreguidelines-const-correctness" -header-filter=".*" reproconst.cpp -- -std=c++17 -Wall
54 warnings generated.
reproconst.cpp:22:9: warning: variable 'anInstanceOf' of type '<dependent type>' can be declared 'const' [cppcoreguidelines-const-correctness]
        const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
        ^
                       const
Suppressed 53 warnings (53 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

I can not reproduce that case :(
It gives me a compilation error in const DoGooder anInstanceOf..., because DoGooder requires an template parameter. I then add DoGooder<int> and the variable gets no transformation.
Did you reproduce the error with exactly that code?

And which version did you run? Maybe that was a previous false positive, that might be fixed right now?

0x8000-0000 added a comment.EditedSat, Jan 11, 6:54 AM

Here is a minimal example that shows the problem:

I can not reproduce that case :(
It gives me a compilation error in const DoGooder anInstanceOf..., because DoGooder requires an template parameter. I then add DoGooder<int> and the variable gets no transformation.
Did you reproduce the error with exactly that code?

And which version did you run? Maybe that was a previous false positive, that might be fixed right now?

The way I am testing is that I am fetching the current master branch of https://github.com/llvm/llvm-project.git, then downloading the latest patch from here and applying on top, then building.

This is the state of the tree right as of last night:

commit 1c8ab48028cc39429a392691ef2e66968460d782 (HEAD -> D54943.diff12)
Author: Florin Iucha <florin@signbit.net>
Date:   Fri Jan 10 18:52:54 2020 -0500

    D54943.diff12

commit 1b8c84b8dd5a4a294943a6a6f0631d2d3a1f9f27 (origin/master, origin/HEAD, master)
Author: Richard Smith <richard@metafoo.co.uk>
Date:   Fri Jan 10 15:47:29 2020 -0800

    Improve precision of documentation comment.

CTAD takes care of the missing template parameter, that's why I'm passing "-std=c++17" to clang-tidy.

/tmp$ /opt/clang10/bin/clang-tidy -checks="-*,cppcoreguidelines-const-correctness" -header-filter=".*" test.cpp -- -std=c++17 -Wall
54 warnings generated.
/tmp/test.cpp:22:9: warning: variable 'anInstanceOf' of type '<dependent type>' can be declared 'const' [cppcoreguidelines-const-correctness]
        const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
        ^
                       const 
Suppressed 53 warnings (53 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
/tmp$ /opt/clang10/bin/clang++ -c test.cpp
test.cpp:22:15: error: use of class template 'DoGooder' requires template arguments
        const DoGooder anInstanceOf{nullptr, Foo::someRandomConstant};
              ^
test.cpp:4:8: note: template is declared here
struct DoGooder
       ^
1 error generated.
/tmp$ /opt/clang10/bin/clang++ -std=c++17 -c test.cpp
-rw-r--r-- 1 florin florin 432 Jan 11 09:48 test.cpp
-rw-r--r-- 1 florin florin 744 Jan 11 09:52 test.o

And just for completeness:

/tmp$ /opt/clang10/bin/clang++ -v
clang version 10.0.0 (https://github.com/llvm/llvm-project.git 1c8ab48028cc39429a392691ef2e66968460d782)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /opt/clang10/bin

The reported git version is the one from the top of the log.

JonasToth updated this revision to Diff 237501.Sat, Jan 11, 8:05 AM
  • remove pointee-analysis artifacts as this will be done later
  • remove unnecessary comments
JonasToth updated this revision to Diff 237503.Sat, Jan 11, 8:24 AM
  • fix false positive with ignoring dependentTypes

@0x8000-0000 i did remove dependentTypes from matching. Locally that works with your reported test-case (i was running clang-tidy wrong :().

I do understand the auto & issues now (i think).
If the type is deduced to a template-type or something that depends the standard ways of detecting that do not work, because in each template instantiation this information is not available.

Having a auto & local = TemplatedVariable does not have the isInstantiationDependentType()-bits set and for each instantiation of the template the const-analysis is run.
The false positives happened in different template instantiations, i think because the overloaded operators were sometimes const and sometimes not. I tried many ways to detect, if the type comes from a template, but had no success.
This is probably an issue in Sema or requires adjustments there.
I added tests in clang-tidy, but #if 0 them out because are not working right now.

@0x8000-0000 i did remove dependentTypes from matching. Locally that works with your reported test-case (i was running clang-tidy wrong :().

I do understand the auto & issues now (i think).
If the type is deduced to a template-type or something that depends the standard ways of detecting that do not work, because in each template instantiation this information is not available.

Having a auto & local = TemplatedVariable does not have the isInstantiationDependentType()-bits set and for each instantiation of the template the const-analysis is run.
The false positives happened in different template instantiations, i think because the overloaded operators were sometimes const and sometimes not. I tried many ways to detect, if the type comes from a template, but had no success.
This is probably an issue in Sema or requires adjustments there.
I added tests in clang-tidy, but #if 0 them out because are not working right now.

Thank you for looking into this; I'll start building diff14 to test locally.

I'm good with merging this checker as-is, as long as it is not enabled by default. I want to be able to use it even partially, in environments where I might not be able to inject a home-built compiler ;)

Thank you for looking into this; I'll start building diff14 to test locally.

Thanks :)

I'm good with merging this checker as-is, as long as it is not enabled by default.

Which part shall be disabled in your opinion? The whole check? that would not work xD But I think both value and reference analysis is good enough. The issue with auto& is only a problem in templates, where different instantiations result to different modification-status if the type of the reference depends on the template instantiation.
Its not that common, but a false positive. :(

But i think it is already valueable in day-to-day programming and to modernize code, lets see what aaron says :)

I have built diff14 and tried to apply it on an internal code base with ~7500 translation units.

$ python3 /opt/clang10/share/clang/run-clang-tidy.py -clang-tidy-binary  /opt/clang10/bin/clang-tidy -clang-apply-replacements /opt/clang10/bin/clang-apply-replacements -p ../build.clang10.dbg/ "-checks=-*,cppcoreguidelines-const-correctness"  -fix -j 24 -quiet -deduplicate $(find lib -name \*.cpp) > const_fix.log 2&> const_err.log

The diffstat is

1608 files changed, 19927 insertions(+), 19927 deletions(-)

This generated 56 "const const" declarations, of which 25 were triple const!

I have tried -deduplicate argument as in Jonas' script, but ...

run-clang-tidy.py: error: unrecognized arguments: -deduplicate

Now onto editing those files by hand to remove the duplications and re-running the build.

0x8000-0000 added a comment.EditedSat, Jan 11, 8:38 PM

Summary of "misses"

One:

uint32_t counter = 0;
BOOST_FOREACH(const Foo* foo, *theFoos)
{
    if (foo->getType() == someType)
    {
        ++counter;
    }
}

The -fix makes the counter const :), so obviously it breaks compilation.

Two:

It marks a lot of std::stringstream / std::ostringstream instances as const. So we can't << into it, breaking compilation.

Three:

It marked an array in the header as const. There were some pointer variables (in a translation unit which included that header) that were declared and initialized to 0 (meaning nullptr), then later in a case block of a switch they were assigned &array[0] value. It did not change the pointer variables, but they used to be mutable pointer to mutable, so now the assignment from a const array failed to build.

The changes required to get the build complete (7500 translation units);

15 files changed, 59 insertions(+), 59 deletions(-)

I can live with the breakage in the 3rd case - since the fix is one time and straightforward (and makes the code clearly better). But I prefer not to have to annotate all std::sotringstream instances with NOLINT. Also we have a fair number of BOOST_FOREACH macros from way back then. They should be ripped out of course, but it's in code that wasn't touched in a while, so having the clang-tidy keep reporting / misfixing that area is bothersome.

I really want this clang-tidy tool, but I don't want to put off users via false positives. Many don't want to deal with it anyway, and the smallest excuse will be brought up against enabling the use of the tool.

0x8000-0000 added a comment.EditedSat, Jan 11, 9:22 PM

One more mis-constification that I can't explain, nor reduce to a small test case (when I extract the code, the check behaves as expected / desired)

namespace util {
  struct StringIgnoreInitialHash : public std::unary_function<std::string, size_t>
  {
     size_t operator()(const std::string& rhs) const
     {
        std::size_t hash = 0;
        std::string::const_iterator str = rhs.begin();
        std::string::const_iterator end = rhs.end();
        if (str != end)
        {
           boost::hash_combine(hash, std::toupper(*str, std::locale()));
           ++str;
        }
        for (; str != end; ++str)
        {
           boost::hash_combine(hash, *str);
        }
        return hash;
     }
  };
}

This is in a header included 29 times in other headers and 106 times directly in translation units.

The const-checker reported 31 times that the variable 'hash' can be made constant.

Also it reported 5 times that the variable 'str' can be made constant (which it can't) and 194 times that variable 'end' can be made constant (which it can, and should).

Running in fix mode, made 'hash', 'str' and 'end' all constants.

Extracting this into its own translation unit and adding the requisite \#includes and paths to the boost files does not reproduce the error. Only 'end' is, correctly, reported as needing to be const.

As an aside, once this is merged in, I dream of a "fix-it" for old style C code:

int foo;

...
/* page of code which does not use either foo or bar */
...
foo = 5;

Moves the foo declaration to the line where it is actually first initialized. Then run the constify tool on top.

As an aside, once this is merged in, I dream of a "fix-it" for old style C code:

int foo;

...
/* page of code which does not use either foo or bar */
...
foo = 5;

Moves the foo declaration to the line where it is actually first initialized. Then run the constify tool on top.

You are not first :-) See PR21983.

As an aside, once this is merged in, I dream of a "fix-it" for old style C code:

int foo;

...
/* page of code which does not use either foo or bar */
...
foo = 5;

Moves the foo declaration to the line where it is actually first initialized. Then run the constify tool on top.

You are not first :-) See PR21983.

There's https://reviews.llvm.org/D64671 which solves half the problem.

This generated 56 "const const" declarations, of which 25 were triple const!

I've tried this on ccache (326 fixes, nice!), but some came out as 'const const'.
At least in my case I traced it back to having src/...... and ./src/...... within the fixes.yml file.
On first glance these result from having ./src as an include directory.

After replacing -I./ with -I within compile_commands.json I'm not getting const const anymore.

Not sure who is at fault here / who should fix it.
But at least in the trivial case of ./ I feel clang-tidy should fix it when it writes the yml files?!

This surfaces here simply due to the shire number of files with fixes.