Page MenuHomePhabricator

[clang-tidy] implement new check for const-correctness
Needs ReviewPublic

Authored by JonasToth on Apr 9 2018, 9:05 AM.

Details

Summary

This check aims to determine values and references that could be declared
as const, but are not.

The first version I am aiming at only analyzes local variables.
No fixits are emitted.

This check aims to implement CPPCG-ES. 25: Declare an object const or constexpr unless you want to modify its value later on
and HICPP-7.1.2 Use const whenever possible.

Because const-correctness includes many issues this check will be implemented
in multiple stages.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
JonasToth updated this revision to Diff 158285.Jul 31 2018, 8:34 AM
  • Merge branch 'master' into check_const
JonasToth updated this revision to Diff 159062.Aug 3 2018, 11:48 AM

get check up to 8.0

I'm sorry for the delay in reviewing this; I'm not certain how it fell off my radar for so long!

clang-tidy/cppcoreguidelines/ConstCheck.cpp
32 ↗(On Diff #159062)

Do you intend to support Obj-C object pointers as well?

141–142 ↗(On Diff #159062)

Why is this check disabled for C code?

149 ↗(On Diff #159062)

Drop the comma; that -> which

152–153 ↗(On Diff #159062)

Can this use unless(anyOf(...)) instead?

164–165 ↗(On Diff #159062)

No need for this -- check() won't be called unless there are registered matchers.

167 ↗(On Diff #159062)

Can you pick a slightly different name -- Scope is the name of a type in the clang namespace.

190 ↗(On Diff #159062)

const -> 'const'

clang-tidy/cppcoreguidelines/ConstCheck.h
25 ↗(On Diff #159062)

Grammar nit, perhaps: This check warns on variables which could be declared const but are not.

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
45

This name leaves a bit to be desired. How about cppcoreguidelines-const-correctness?

docs/clang-tidy/checks/cppcoreguidelines-const.rst
6 ↗(On Diff #159062)

that -> which

8 ↗(On Diff #159062)

coding guidelines for example -> coding guidelines, such as:

12 ↗(On Diff #159062)

type based -> type-based

I'm sorry for the delay in reviewing this; I'm not certain how it fell off my radar for so long!

No problem :)

clang-tidy/cppcoreguidelines/ConstCheck.cpp
32 ↗(On Diff #159062)

For now not, because I have no experience nor knowledge with Obj-C.

xbolva00 added inline comments.
clang-tidy/cppcoreguidelines/ConstCheck.cpp
28 ↗(On Diff #159062)

typo
paramters -> parameters

JonasToth updated this revision to Diff 159220.Aug 5 2018, 8:48 AM
JonasToth marked 11 inline comments as done.
  • Merge branch 'master' into check_const
  • [Misc] rename and first review comments
  • language stuff
JonasToth added inline comments.Aug 5 2018, 8:48 AM
clang-tidy/cppcoreguidelines/ConstCheck.cpp
141–142 ↗(On Diff #159062)

No actual reason. I will allow it for C, too.

JonasToth updated this revision to Diff 159221.Aug 5 2018, 8:51 AM
  • doc list
JonasToth updated this revision to Diff 159224.Aug 5 2018, 9:14 AM
  • revert breaking change in LocalVar matcher.

Reverting the change from allOf(...) to unless(anyOf(..)). I did not investigate
the reason for it breaking, because basic logic suggests that transformation
should be correct.

JonasToth added inline comments.Aug 5 2018, 9:15 AM
clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
148

@aaron.ballman The change was not valid for some reason. I leave it like it is if thats ok with you.

JonasToth updated this revision to Diff 159225.Aug 5 2018, 9:19 AM
JonasToth marked an inline comment as done.
  • fix typo
JonasToth marked an inline comment as done.Aug 5 2018, 9:20 AM
JonasToth updated this revision to Diff 159226.Aug 5 2018, 9:27 AM
  • update ReleaseNotes

The functionality is looking good, aside from a few small nits remaining. However, I'm wondering how this should integrate with other const-correctness efforts like readability-non-const-parameter? Also, I'm wondering how this check performs over a large code base like LLVM -- how chatty are the diagnostics, and how bad is the false positive rate (roughly)?

clang-tidy/cppcoreguidelines/ConstCheck.cpp
32 ↗(On Diff #159062)

Okay, then please add a comment mentioning that they're explicitly not handled yet (perhaps with a FIXME).

clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
148

That's.... really odd. I am fine leaving it as-is for this patch, but it would be good to understand why that code fails as it seems like a reasonable exposition.

184

Still missing the single quotes around const in the diagnostic.

JonasToth marked 3 inline comments as done.Aug 6 2018, 1:57 PM
JonasToth marked an inline comment as done.Aug 6 2018, 2:01 PM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
148

Added a TODO. But maybe i did the transformation incorrectly too. Not all conditions are negative. Maybe all the negative ones can be inverted by demorgan. That should be correct.

I will check this out later.

JonasToth marked an inline comment as done.Aug 6 2018, 2:02 PM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
184

Ups. The comment has them :D

JonasToth updated this revision to Diff 159390.Aug 6 2018, 2:03 PM
JonasToth marked 4 inline comments as done.
  • address review issues, todos/fixmes and diag nit

However, I'm wondering how this should integrate with other const-correctness efforts like readability-non-const-parameter?

I think this check/functionality will kinda replace the readability-non-const-parameter check. The readability check does not a full const-analysis too and i think only works on pointers or sth like this.
Maybe the check name will still exist, but use the ExprMutAnalyzer or it will become an alias to this with special configuration.
I would like to add support for marking methods const plus the ability for code transformation. Currently looking into clang-refactor framework to implement general refactoring primitives necessary for that.
In general its probably better to have one check, that handles all const issues.

Also, I'm wondering how this check performs over a large code base like LLVM -- how chatty are the diagnostics, and how bad is the false positive rate (roughly)?

I will prepare a report for this tomorrow. Currently the LLVM builds take very long on my laptop :(

For reference, here is the output for llvm/lib.

Things i noticed:

  • lambdas are warned as potential const, i think they should be excluded for the values
  • for-loops that initialize two values (usually the start and end-iterator) are correctly diagnosed, but the diagnosis might be misleading. Especially because there is no way to have a const-iterator initialized together with the changing iterator.

for (auto begin = vec.begin(), end = vec.end(); ...)

  • there seems to be a false positive with array-to-pointer decay. ExprMutAnalyzer does think of it, but maybe there is a bug in it.
  • pointer diagnostics don't seem to worker (pointer-as-value). The test-case does work, This must be analyzed further
  • there was a bug in the check method, where AnalyzeValues == 0 did imply no analysis is done.

Given the volume of diagnostics for the value-case i did not investigate many cases manually. I think it makes sense to strive for code-transformation and check if the code still compiles.

References on the other hand seem to function correctly, and most references are single-var-decls, too. That means code transformation is very simple and therefor an easy target to check functionality.
References and values are in general treated the same, so it might give a hint about the performance for values.

  • there seems to be a false positive with array-to-pointer decay. ExprMutAnalyzer does think of it, but maybe there is a bug in it.

Could you give a concrete example of this?

Could you give a concrete example of this?

vi llvm/lib/Demangle/ItaniumDemangle.cpp +1762

/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1762:7: warning:
variable 'num' of type 'char [FloatData<Float>::max_demangled_size]' can
be declared 'const' [cppcoreguidelines-const-correctness]
      char num[FloatData<Float>::max_demangled_size] = {0};
     ^
/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1763:7: warning:
variable 'n' of type 'int' can be declared 'const'
[cppcoreguidelines-const-correctness]
      int n = snprintf(num, sizeof(num), FloatData<Float>::spec, value);
     ^

JonasToth updated this revision to Diff 159775.Aug 8 2018, 1:33 PM
  • fix bug with AnalyzeValues==false skipping whole check, adjust test code to 'const' instead of const

Could you give a concrete example of this?

vi llvm/lib/Demangle/ItaniumDemangle.cpp +1762

/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1762:7: warning:
variable 'num' of type 'char [FloatData<Float>::max_demangled_size]' can
be declared 'const' [cppcoreguidelines-const-correctness]
      char num[FloatData<Float>::max_demangled_size] = {0};
     ^
/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1763:7: warning:
variable 'n' of type 'int' can be declared 'const'
[cppcoreguidelines-const-correctness]
      int n = snprintf(num, sizeof(num), FloatData<Float>::spec, value);
     ^

Looks like related to template.
the exact type of num depends on Float
and the CallExpr is not even resolved because we don't know the type of the arguments yet (adl, overload resolution, etc.)
So the AST doesn't contain any array to pointer decay.

I guess we should suppress the check in such cases.

Always the same with the templates ;) So uninstantiated templates should
just be ignored.

I think it would be better to have it in the ExprMutAnalyzer, because
that part can not decide on const-ness. Fixing it here would just
circumvent the problem but not fix it, would you agree?

Am 10.08.2018 um 22:12 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45444#1191874, @JonasToth wrote:

lCould you give a concrete example of this?

vi llvm/lib/Demangle/ItaniumDemangle.cpp +1762

/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1762:7: warning:

variable 'num' of type 'char [FloatData<Float>::max_demangled_size]' can
be declared 'const' [cppcoreguidelines-const-correctness]
      char num[FloatData<Float>::max_demangled_size] = {0};
     ^
/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1763:7: warning:
variable 'n' of type 'int' can be declared 'const'
[cppcoreguidelines-const-correctness]
      int n = snprintf(num, sizeof(num), FloatData<Float>::spec, value);
     ^

Looks like related to template.
the exact type of num depends on Float
and the CallExpr is not even resolved because we don't know the type of the arguments yet (adl, overload resolution, etc.)
So the AST doesn't contain any array to pointer decay.

I guess we should suppress the check in such cases.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

JonasToth updated this revision to Diff 160239.Aug 11 2018, 3:51 AM
  • explicitly ignore lambdas in VarDecls

Always the same with the templates ;) So uninstantiated templates should
just be ignored.

I think it would be better to have it in the ExprMutAnalyzer, because
that part can not decide on const-ness. Fixing it here would just
circumvent the problem but not fix it, would you agree?

Agreed :)
I'll create a diff handling various template related cases in ExprMutationAnalyzer.

Thank you very much :)

Am 12.08.2018 um 00:17 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45444#1196271, @JonasToth wrote:

Always the same with the templates ;) So uninstantiated templates should

just be ignored.

I think it would be better to have it in the ExprMutAnalyzer, because

that part can not decide on const-ness. Fixing it here would just
circumvent the problem but not fix it, would you agree?

Agreed :)
I'll create a diff handling various template related cases in ExprMutationAnalyzer.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

  • get up to date
JonasToth updated this revision to Diff 164898.Sep 11 2018, 8:48 AM
  • ignore lambdas properly


Here are results for llvm/lib with the current version.

I could not find any outstanding issues right now. I feel that the check needs transformation capabilties. Then i would try to produce invalid const transformations to find false positives. Spoting false negatives is harder though.

JonasToth updated this revision to Diff 165497.Sep 14 2018, 7:02 AM
  • update to ExprMutAnalyzer living in clang now

Yeah, it would be super useful if Clang can add const to all places where possible :) love this work, great!

Soonish it might be able to do so ;)

Am 14.09.2018 um 17:13 schrieb Dávid Bolvanský via Phabricator:

xbolva00 added a comment.

Yeah, it would be super useful if Clang can add const to all places where possible :) love this work, great!

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

  • fix actually use clang-analyses correctly

Are you still looking at this?

I am, but this revision relies on support on the analysis in the
frontend. This seems currently stalled as the developer there seems busy
with other stuff.

Once i implement transofmration-capabilites i will evaluate if there are
any false-positives that would break. Once that is done it might still
land even if there is no frontend progress.

Right now we are aware of false-negatives in the const-ness detection,
that should not influence this check though. And of course obscure
(template) cases might still be buggy.

Am 31.10.18 um 20:33 schrieb Dávid Bolvanský via Phabricator:

xbolva00 added a comment.

Are you still looking at this?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

JonasToth updated this revision to Diff 173653.Nov 12 2018, 4:33 AM
  • Merge branch 'master' into check_const
JonasToth updated this revision to Diff 174942.Nov 21 2018, 9:17 AM
  • Merge branch 'master' into check_const
JonasToth updated this revision to Diff 175445.Nov 27 2018, 3:45 AM
  • Merge branch 'master' into check_const
  • avoid bitrot