Page MenuHomePhabricator

WIP [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
JonasToth added inline comments.Jan 4 2020, 10:47 AM
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.Jan 4 2020, 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.Jan 4 2020, 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.Jan 5 2020, 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.Jan 5 2020, 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.Jan 5 2020, 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.EditedJan 5 2020, 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.Jan 5 2020, 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.Jan 6 2020, 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.Jan 8 2020, 2:41 PM
JonasToth added inline comments.
clang/lib/Analysis/ExprMutationAnalyzer.cpp
20
JonasToth added a comment.EditedJan 8 2020, 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.Jan 9 2020, 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.Jan 10 2020, 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.Jan 10 2020, 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.EditedJan 10 2020, 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.EditedJan 10 2020, 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.EditedJan 11 2020, 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.Jan 11 2020, 8:05 AM
  • remove pointee-analysis artifacts as this will be done later
  • remove unnecessary comments
JonasToth updated this revision to Diff 237503.Jan 11 2020, 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.EditedJan 11 2020, 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.EditedJan 11 2020, 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.

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

This could be due to multiple instantiations of a template, that each created a fix. this is an issue I have to work on.
Otherwise it could come from header files and the fix is not deduplicated in clang-apply-replacements (which should deduplicate, I believe).
The path issues come most likely from there and should be fixed there.

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

Overall the check is almost done, but the last bit I am struggling with. I want no false positives, because I assume it will be used more often and creates so many fixes that fixing false positives is very cumbersome.

McKeck added a subscriber: McKeck.Jul 18 2020, 2:10 PM

Observed behavior:

Change: for(string_view token : split_into_views(file_content, " \t\r\n")) --> for(string_view const token : split_into_views(file_content, " \t\r\n")).
Note: std::vector<string_view> split_into_views(string_view input, const char* separators);
Problem: Now it triggers error: loop variable 'token' of type 'const nonstd::sv_lite::string_view' (aka 'const basic_string_view<char>') creates a copy from type 'const nonstd::sv_lite::string_view' [-Werror,-Wrange-loop-analysis]
Fixed manually by making it for(std::string_view& const token : split_into_views(file_content, " \t\r\n")).

connojd added a subscriber: connojd.Thu, Sep 3, 8:49 PM
JonasToth updated this revision to Diff 293392.Tue, Sep 22, 2:59 AM
  • rebase to master after AST Matchers are extracted

Observed behavior:

Change: for(string_view token : split_into_views(file_content, " \t\r\n")) --> for(string_view const token : split_into_views(file_content, " \t\r\n")).
Note: std::vector<string_view> split_into_views(string_view input, const char* separators);
Problem: Now it triggers error: loop variable 'token' of type 'const nonstd::sv_lite::string_view' (aka 'const basic_string_view<char>') creates a copy from type 'const nonstd::sv_lite::string_view' [-Werror,-Wrange-loop-analysis]
Fixed manually by making it for(std::string_view& const token : split_into_views(file_content, " \t\r\n")).

Hey alex,
thanks for testing the check out and reporting the issue.
Could you please provide me a full reproducer (optimally without dependencies on includes/libraries)?
I am currently trying to go through everything again, as there are simply some cases not handled correctly :/

Best Regards, Jonas

JonasToth updated this revision to Diff 293418.Tue, Sep 22, 5:12 AM
  • include ExprMutAnalyzer implementation changes, that got lost somehow with prior rebase
JonasToth updated this revision to Diff 293422.Tue, Sep 22, 5:21 AM
  • remove spurious formatting change
JonasToth retitled this revision from [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness to WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness.Tue, Sep 22, 8:07 AM

Master branch has too many false positives for tidy - would it be possible to create a branch that contains this patch set on top of llvm-11.0-rc3? I would then add this to our internal CI.

For the legacy code (see previous reports) we found quite a few false positives. For a new code base that we're developing from scratch with C++20, there have been no false positives - it catches missed 'const' much better than human reviewers - we still keep a Clang10 around that I built from source with the previous version of the patches :)

Master branch has too many false positives for tidy - would it be possible to create a branch that contains this patch set on top of llvm-11.0-rc3? I would then add this to our internal CI.

Yeah, it is not that simple to get this check into something really usefull :/
I can create a branch in my own fork, that would need a little work, which i can do in ~8 hours. I would then ping you.

For the legacy code (see previous reports) we found quite a few false positives. For a new code base that we're developing from scratch with C++20, there have been no false positives - it catches missed 'const' much better than human reviewers - we still keep a Clang10 around that I built from source with the previous version of the patches :)

It would be great if you could keep testing your legacy code-base once this patch progresses even further.
It is a great message, that the modern codebase does not make big trouble :) Given I tested mostly on LLVM itself so far, this is not too surprising as its a modern codebase itself.

Master branch has too many false positives for tidy - would it be possible to create a branch that contains this patch set on top of llvm-11.0-rc3? I would then add this to our internal CI.

Yeah, it is not that simple to get this check into something really usefull :/

Just to be clear, the false positives I was referring to are not in this checker, but other checkers (uninitialized variables mostly) that were committed to the origin/main branch.

I can create a branch in my own fork, that would need a little work, which i can do in ~8 hours. I would then ping you.

Thank you - I tried cherry-picking the changes from the main branch onto the release branch and ... it does not build - didn't have time to debug it - and I am hoping you're more quick at making the adaptation.

For the legacy code (see previous reports) we found quite a few false positives. For a new code base that we're developing from scratch with C++20, there have been no false positives - it catches missed 'const' much better than human reviewers - we still keep a Clang10 around that I built from source with the previous version of the patches :)

It would be great if you could keep testing your legacy code-base once this patch progresses even further.
It is a great message, that the modern codebase does not make big trouble :) Given I tested mostly on LLVM itself so far, this is not too surprising as its a modern codebase itself.

I will keep testing on both codebases - the modern one we're using it in the CI so it gets tested vigorously. Once I get some time I'll take another pass at the legacy code base to see what it finds.

Thank you for resuming the work on this. I was afraid I'm going to lose this check when we have to move off Clang10 - (we're pursuing C++20 aggressively in the new project).

Could you please provide me a full reproducer (optimally without dependencies on includes/libraries)?

I can certainly do that based on my old version from ~9 months ago or preferably if you provide me some branch somewhere (github?). I have trouble applying the latest diff to latest master myself.

Could you please provide me a full reproducer (optimally without dependencies on includes/libraries)?

I can certainly do that based on my old version from ~9 months ago or preferably if you provide me some branch somewhere (github?). I have trouble applying the latest diff to latest master myself.

https://github.com/JonasToth/llvm-project/tree/feature_const_transform is close to the origin/master (a few commits behind).

JonasToth updated this revision to Diff 293970.Thu, Sep 24, 1:36 AM
rebase to current exprmutanalyzer

- remove spurious formatting change
- fix include and typo

@AlexanderLanin @0x8000-0000 i created the branch release-11-const (https://github.com/JonasToth/llvm-project/tree/release-11-const) in my fork.

This branch is based on 11-rc3 and cherry picks the commits that correspond to this revision.
I hope this is of any use for you! I will probably forget to update this branch all the time, but if I feel like there has been actual progress made, i will update!

@AlexanderLanin @0x8000-0000 i created the branch release-11-const (https://github.com/JonasToth/llvm-project/tree/release-11-const) in my fork.

This branch is based on 11-rc3 and cherry picks the commits that correspond to this revision.
I hope this is of any use for you! I will probably forget to update this branch all the time, but if I feel like there has been actual progress made, i will update!

Thank you Jonas - will add this to our CI tool for the new project and try to find some time to run it on the legacy project.

"Minimal" example for the behavior I described before.

clang++ -fsyntax-only ~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp -Wrange-loop-analysis 2>&1 | wc -l
269

Then fix it by:
~/llvm/build-const-fix/bin/clang-tidy ~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp -checks="-*,cppcoreguidelines-const-correctness" -fix

And recount:
clang++ -fsyntax-only ~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp -Wrange-loop-analysis 2>&1 | wc -l
283

Yet again I want to mentioned that this is technically not a bug, but in combination with Werror it's not ideal. Basically I cannot compile my codebase after this fixit since Werror is set. Although the code is arguably better.

Off-Topic: I was just attempting to apply this to my codebase at work.
Seems this will be more challenging than anticipated 😉
My best guess is this is related to D72730

Applying fixes ...
terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc

Off-Topic: I was just attempting to apply this to my codebase at work.
Seems this will be more challenging than anticipated 😉
My best guess is this is related to D72730

Applying fixes ...
terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc

Lets start with this one:
What did you exactly do?
Is this a run of run-clang-tidy for a whole codebase?
In the end, it could actually just boil down to "not enough ram", because clang-apply-replacements do merge the diagnostics and stuff at some point.
Could you please provide more information about your issue here so that it is possible to root out other issues than memory-limitations. Even those should be addressed in some way.