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
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.Sep 3 2020, 8:49 PM
JonasToth updated this revision to Diff 293392.Sep 22 2020, 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.Sep 22 2020, 5:12 AM
  • include ExprMutAnalyzer implementation changes, that got lost somehow with prior rebase
JonasToth updated this revision to Diff 293422.Sep 22 2020, 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.Sep 22 2020, 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.Sep 24 2020, 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.

@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.

Just a quick update; in clang-tidy mode it works great, with no false positive and no crashes. I haven't tried the 'apply-replacements' mode.

JonasToth updated this revision to Diff 296549.Tue, Oct 6, 2:35 PM

rebase to newest expmutanalyzer patch

@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.

Just a quick update; in clang-tidy mode it works great, with no false positive and no crashes. I haven't tried the 'apply-replacements' mode.

That is great to hear! I update the release-11-const branch with the current state.

If you try the replacement-mode (with run-clang-tidy) beware, that there is a bug in the deduplication of clang-apply-replacements and some templated functions result in multiple applications of const.
This is something i need to work on, before everything can land :)

aaron.ballman added inline comments.Thu, Oct 8, 7:49 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
32

Why do serialization and lex need to be pulled in?

clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
47

Should this check only fire in C++? I'm sort of on the fence. It's a C++ core guideline, so it stands to reason it should be disabled for C code. But const-correctness is a thing in C too. WDYT?

106

can only in -> can only be in

JonasToth updated this revision to Diff 297197.Fri, Oct 9, 5:09 AM
  • update to landed ExprMutAnalyzer changes
JonasToth marked 2 inline comments as done.Fri, Oct 9, 6:02 AM
JonasToth added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
32

Thats a good question 🤔
Probably old artifact from where everything was done here.
Removed everything except analysis

clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
47

I do not know all the subtle differences between C and C++ here. Judging from this: https://stackoverflow.com/questions/5248571/is-there-const-in-c the current form would not be correct for C for pointers.
The assumptions of this check and especially the transformations are done for C++. I dont see a reason why the value-semantic would not apply for both.

Maybe there is a way to make the code compatible for both languages. The easiest solution would probably to not do the 'pointer-as-value' transformation. This is not that relevant as a feature anyway. I expect not nearly as much usage of this option as for the others.

In the end of the day i would like to support both C and C++. Right now it is untested and especially the transformation might break the code. It should run on both languages though, as there is no language checking.
I will add some real world C code-bases for the transformation testing and see what happens :)

JonasToth updated this revision to Diff 297215.Fri, Oct 9, 6:03 AM
JonasToth marked an inline comment as done.
  • address review comments
JonasToth updated this revision to Diff 297224.Fri, Oct 9, 6:43 AM
  • fix platform dependent warning message for decltype to just match the beginning and not the 'a.k.a'
JonasToth updated this revision to Diff 297231.Fri, Oct 9, 7:23 AM
  • missed one place of decltype
aaron.ballman added inline comments.Fri, Oct 9, 12:38 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
47

Judging from this: https://stackoverflow.com/questions/5248571/is-there-const-in-c the current form would not be correct for C for pointers.

Sure, there may be additional changes needed to support the other oddities of C. I was asking more at the high level.

In the end of the day i would like to support both C and C++. Right now it is untested and especially the transformation might break the code.

Another option is for us to restrict the check in its current form to just C++ and then expose a C check from it in a follow-up (likely under a different module than cppcodeguidelines). For instance, CERT has a recommendation (not a rule) about this for C (https://wiki.sei.cmu.edu/confluence/display/c/DCL00-C.+Const-qualify+immutable+objects) and I would not be surprised to find it in other coding standards as well.

clang-tools-extra/docs/ReleaseNotes.rst
167

This looks unintentional to me.

JonasToth updated this revision to Diff 297405.Sat, Oct 10, 5:11 AM
  • no delayed template parsing
  • adjust release note issue
JonasToth marked an inline comment as done.Sat, Oct 10, 5:12 AM
JonasToth added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
47

Another const check is probably better. The clang-tidy part should be minimal effort. I think there isn't alot of duplication either.
We could still think of proper configurability of this check and alias it into other modules with proper defaults for C.

JonasToth updated this revision to Diff 297407.Sat, Oct 10, 5:43 AM
  • fix test RUN line
aaron.ballman added inline comments.Tue, Oct 13, 11:10 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
47

Okay, I'm sold -- can you not register the check unless in C++ mode? We can add a C-specific check later.

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

Here's another terrible one to try -- can you make sure we don't try to make something const when the variable is evaluated in a context where it's usually not:

int N = 1; // Can't make N 'const' because VLAs make everything awful
sizeof(int[++N]);
924

Rather than comment out the code, I think it makes more sense to leave the code here, CHECK for the existing diagnostics, but add a FIXME comment to any diagnostics (or lack of diagnostics) that are incorrect. It makes it more clear what the current behavior is and what we want it to eventually be.

Would it be possible to split this review further, into "checking" and "fixing" portions? I understand that fix-it portion is more difficult, and sometimes results in multiple 'const' keywords being added, but for the past year we have used the check as part of regular CI runs to flag where it needs to be added by the engineers and for our use case it works fine that way - so I would prefer to have at least the "report" part as part of upstream Clang12, even if the 'fixup' comes later, due to the complexity of covering all corner cases.

Would it be possible to split this review further, into "checking" and "fixing" portions? I understand that fix-it portion is more difficult, and sometimes results in multiple 'const' keywords being added, but for the past year we have used the check as part of regular CI runs to flag where it needs to be added by the engineers and for our use case it works fine that way - so I would prefer to have at least the "report" part as part of upstream Clang12, even if the 'fixup' comes later, due to the complexity of covering all corner cases.

I wouldn't have an issue with that.