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

[JonasToth]

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.

[JonasToth]

@aaron.ballman, @lebedev.ri, @alexfh and everyone else, i would love to hear your opinion on my approach. i never implemented a similar check and i might miss some cases, that need to be considered.

The current state is just a proof-of-concept to see how such a check could be implemented.

[Eugene.Zelenko]

It'll be good idea to have option to apply this check for pointer/references only, or include built-in types/enums.

clang-tidy/cppcoreguidelines/ConstCheck.cpp

13 ↗

(On Diff #141668)

Please remove empty line.

docs/ReleaseNotes.rst

70

Please move into new checks list in alphabetical order.

73

Please add short description here.

docs/clang-tidy/checks/cppcoreguidelines-const.rst

6 ↗

(On Diff #141668)

Preliminary documentation will be helpful.

[Eugene.Zelenko]

Will be good idea to add HICPP alias.

[JonasToth]

It'll be good idea to have option to apply this check for pointer/references only, or include built-in types/enums.

Agreed. I aim at a mark handles const(default on), mark values const(default on for objects), mark pointer (const int * >> const <<) const (default off)

What do you mean by built-in types/enums? That they are configurable separate to objects?

[Eugene.Zelenko]

In D45444#1063291, @JonasToth wrote:

It'll be good idea to have option to apply this check for pointer/references only, or include built-in types/enums.

Agreed. I aim at a mark handles const(default on), mark values const(default on for objects), mark pointer (const int * >> const <<) const (default off)

What do you mean by built-in types/enums? That they are configurable separate to objects?

I meant integers/floating point numbers/Boolean/enums. It may be overkill to apply this check for legacy code base in single pass.

[lebedev.ri]

Thank you for working on this check!

clang-tidy/cppcoreguidelines/ConstCheck.h

60 ↗

(On Diff #141668)

I'm guessing these should be llvm::DenseMap.

[JonasToth]

• [Feature] implement array and method access
• [Feature] finalize value semantic

[JonasToth]

• [Feature] start working on handle semantic

[JonasToth]

• [Misc] unify handle and value modification detection
• [Misc] found new caveats, maybe take a look at refactoring first an require this check to produce clean compiles on llvm

[JonasToth]

@aaron.ballman @lebedev.ri The check is getting really complex. I run it over LLVM and detected some warnings, where iam not sure if they are valid or not. Its already a somewhat usable state, but its hard to determine false positives and false negatives.

For me, that false positives are worse for now, because they annoy. Its easier to add false negatives especially with user feedback. My idea is now: Take a look at refactoring the code to introduce const as fixit and iterate the check until LLVM compiles after the check fixed all missing consts. What do you think about that strategy? And could you take a look at the todo list at the top of ConstCheck.cpp. Did I miss something?

[shuaiwang]

Coming from https://reviews.llvm.org/D45679, which I should have sent out much earlier to get in front of you :p

Kidding aside this check is more mature than mine and I'm happy to help incorporate mine into this one.

I do have a strong opinion that requires non-trivial refactoring of your diff though: having a standalone reusable helper function like isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

• performance/{ForRangeCopyCheck, UnnecessaryCopyInitialization} are using a much less sophisticated isOnlyUsedAsConst(), and can benefit from a more powerful one.
• I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".
• It's cleaner to follow (non-const) reference chains and only mark a decl as "can't be const" if everything on the chain is not modified, avoiding false negatives when a variable is bound to a never-modified non-const reference.
• It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10" modifies "v". This one is particularly tricky because the modifying behavior happens at "c = 10" while the variable we'd like to mark as "can't be const" is arbitrary layers away, and MemberExpr alone doesn't warrant marking "can't be const" because that'll introduce way too many false negatives, ... and I haven't figure out a way to write a matcher to match memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(... <arbitrary layers deep> ... (VarDeclRef) ...), not to mention any layer could either member access or array subscript access.

[JonasToth]

Coming from https://reviews.llvm.org/D45679, which I should have sent out much earlier to get in front of you :p

Kidding aside this check is more mature than mine and I'm happy to help incorporate mine into this one.

I would love to, yours is more elegant :)

I do have a strong opinion that requires non-trivial refactoring of your diff though: having a standalone reusable helper function like isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

Yes please!

performance/{ForRangeCopyCheck, UnnecessaryCopyInitialization} are using a much less sophisticated isOnlyUsedAsConst(), and can benefit from a more powerful one.

I did not think about such possibilities, but maybe other analysis task could benefit too? Warnings?

I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".

Using my approach, you can check if any member variable is used as non-const. Then mark this method as const, if it is not virtual.
Similar for member variables: private non-consts can be converted into consts.

It's cleaner to follow (non-const) reference chains and only mark a decl as "can't be const" if everything on the chain is not modified, avoiding false negatives when a variable is bound to a never-modified 
non-const reference.

For warning only yes. But i want automatic code transformation which will break such code if there are non-const references taken. Having it in one pass is certainly nice. With my approach multiple runs are required if the handles are marked as const.

It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10" modifies "v". This one is particularly tricky because the modifying behavior happens at "c = 10" while the variable we'd like to mark as "can't be const" is arbitrary layers away, and MemberExpr alone doesn't warrant marking "can't be const" because that'll introduce way too many false negatives, ... and I haven't figure out a way to write a matcher to match memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(... <arbitrary layers deep> ... (VarDeclRef) ...), not to mention any layer could either member access or array subscript access.

Does your approach work with the nesting? Maybe something recursive or hasDescendent(modifying) could do it?

Is read your comment on this check later, i first saw your new check. Maybe we should discuss only under one of both :) (which one is not important for me :))

[Eugene.Zelenko]

In D45444#1068496, @shuaiwang wrote:

• I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".

Probably this should be separate check. See also PR21981.

[shuaiwang]

I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".

Using my approach, you can check if any member variable is used as non-const. Then mark this method as const, if it is not virtual.
Similar for member variables: private non-consts can be converted into consts.

You'll also need to check:

• a member function calling another non-const member function -> *this can't be const
• *this is passed as non-const reference param to a function -> *this can't be const

Also when marking decls as "can't be const" we'll have to do record separately for modifying behaviors coming from different functions.
Which of course are all possible but code will get too complex than necessary IMO.

I think member variables are a separate topic: I think we should just treat them as globals and not check on them, the same argument that they can be accessed from multiple translation unit applies to global/namespace scoped variables and class scoped variables. We can only reliably check function/block scope variables.
(reliable meaning being able to achieve zero false positives with useful level of recall, false negatives are inevitable because whenever a modifiable reference/handle escape the current block/translation unit we'll have to assume it's modified.)

It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10" modifies "v". This one is particularly tricky because the modifying behavior happens at "c = 10" while the variable we'd like to mark as "can't be const" is arbitrary layers away, and MemberExpr alone doesn't warrant marking "can't be const" because that'll introduce way too many false negatives, ... and I haven't figure out a way to write a matcher to match memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(... <arbitrary layers deep> ... (VarDeclRef) ...), not to mention any layer could either member access or array subscript access.

Does your approach work with the nesting? Maybe something recursive or hasDescendent(modifying) could do it?

Yes my approach is doing multi-pass matching by calling isModified() recursively. I consider the multi-pass matching "necessary evil" because otherwise we'll have too many false negatives.
I thought about hasDescendent (hasAncestor actually) but I don't think that makes things easier: varDeclRef(hasAncestor(modifying)) would match both "v.a.b.c. = 10" and "map[v] = 10" and we'll still need to double check the modifying behavior does link back to the particular varDeclRef.

Reviewers, what are your thoughts about the approaches?

[shuaiwang]

In D45444#1068967, @Eugene.Zelenko wrote:

In D45444#1068496, @shuaiwang wrote:

• I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".

Probably this should be separate check. See also PR21981.

Sure, a separate check sounds better.
Which makes even strong argument of having a reusable utility checking whether something is modified that can be shared between different checks :)

[JonasToth]

You'll also need to check:

• a member function calling another non-const member function -> *this can't be const
• *this is passed as non-const reference param to a function -> *this can't be const

Yes. That is similar behaviour to checking if a function can be noexcept. But this is all future stuff :)

Also when marking decls as "can't be const" we'll have to do record separately for modifying behaviors coming from different functions.
Which of course are all possible but code will get too complex than necessary IMO.

I cant follow you on that one. I consider every path that allows future modification (like taking a non-const reference/pointer) as cant be const. That would be enough, wouldnt it?
A separate function can only modify a variable if it has some form of non-const handle to it, which must have been created at some point.

I think member variables are a separate topic: I think we should just treat them as globals and not check on them, the same argument that they can be accessed from multiple translation unit applies to global/namespace scoped variables and class scoped variables. We can only reliably check function/block scope variables.
(reliable meaning being able to achieve zero false positives with useful level of recall, false negatives are inevitable because whenever a modifiable reference/handle escape the current block/translation unit we'll have to assume it's modified.)

You are probably right. The only zero-false positive case is: "only const methods called". One could split implementation of a class into several translation units and render the analysis approach useless.

Yes my approach is doing multi-pass matching by calling isModified() recursively. I consider the multi-pass matching "necessary evil" because otherwise we'll have too many false negatives.
I thought about hasDescendent (hasAncestor actually) but I don't think that makes things easier: varDeclRef(hasAncestor(modifying)) would match both "v.a.b.c. = 10" and "map[v] = 10" and we'll still need to double check the modifying behavior does link back to the particular varDeclRef.

Example as reference for later: https://godbolt.org/g/cvhoUN
I will add such cases to my tests.

@shuaiwang Are you in IRC from time to time? I think it will be better to discuss changes in chat, when i start to introduce your approach here.

isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

What i understand from your comments and code:

• implement a utility taking DeclRefExpr and check if there is modification in some scope (Stmt, e.g. CompoundStmt) -> isModified or cantBeModifiedWithin
• match all relevant non-const variable declarations as potential candidates
• track all uses of the candidates and check for modification in their scope

One Note: We could include variables in unnamed namespaces and static declared variables. They have TU scope.

My deviations:

• a variable should be considered modified if a non-const handle is create from it, even if that handle does not modify its referenced value. As first step, those handles should be marked const, then the original value can be marked as const. That is required to produce compiling code after potential code-transformation.

[shuaiwang]

I've updated https://reviews.llvm.org/D45679 and I think the isModified() function there is now sufficiently covering the cases you've covered here and can be used as a good starting version if you plan to use it here.
I copied your const-values test cases and it now passes with the following differences:

• All unused local variables removed, my check will issue a warning for them because technically they're not modified, but I understand why you don't want to cover them. I don't feel strongly about it and I removed it from my copy-pasted test cases because I just to demonstrate isModified()
• Better recall in some_reference_taking, both np_local0 and r1_np_local0 can be caught, similar for np_local1 and non_const_ref in handle_from_array.
• direct_class_access np_local5 triggers with my check (shouldn't it?)
• In range_for non-const loop variables triggers with my check (shouldn't they?)
• In range_for local arrays doesn't trigger with my check, because I'm treating ArrayToPointerDecay the same as taking address of a variable, and looping over an array would involve ArrayToPointerDecay when the implicit __begin/__end are initialized. I added a note inside isModified for future improvements.
• My check over triggers for template (with my failed attempt to fix), but I think it's more of some mistake in the check itself rather than in isModified

Also when marking decls as "can't be const" we'll have to do record separately for modifying behaviors coming from different functions.
Which of course are all possible but code will get too complex than necessary IMO.

I cant follow you on that one. I consider every path that allows future modification (like taking a non-const reference/pointer) as cant be const. That would be enough, wouldnt it?
A separate function can only modify a variable if it has some form of non-const handle to it, which must have been created at some point.

Sorry about the confusion.
Basically consider this example:

class Foo {
public:
  void a() { x = 10; }
  void b() { // nothing }
  void c() { a(); }
  void d() { b(); }
  
private:
  int x;
};

If we check whether isModified(dereference(cxxThisExpr())) for each CompondStmt(hasParent(functionDecl())), we would conclude that:

• a() should stay non-const, because there's this->x = 10 modifying *this
• b() should be changed to const, because nothing modifies *this
• c() should stay non-const, because we're invoking non-const member function on this
• d() should also stay non-const, with the same reason for c(). (We can in theory be smarter about this one because the definition of b() happens to be inside the same TU but that's out of scope right now)

However if we use a per-TU map to record whether x can be const, we'll conclude that x is modified thus can't be const, missing the one that b() is not modifying x. To fix that we'll need two-layered map recording that x is only modified in a() and potentially modified indirectly from c() and d(), so that in the end we can figure out that b() is safe to be marked const.

Anyway, all I was trying to say is: let's use the isModified() approach as it's simpler & cleaner for future use cases like adding const to member functions. And it feels to me that we've already reached agreement there :)

@shuaiwang Are you in IRC from time to time? I think it will be better to discuss changes in chat, when i start to introduce your approach here.

Not really, but I can get myself familiar with it.

isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

What i understand from your comments and code:

• implement a utility taking DeclRefExpr and check if there is modification in some scope (Stmt, e.g. CompoundStmt) -> isModified or cantBeModifiedWithin

It doesn't have to be DeclRefExpr, any Expr should work, and it's useful to make it accept any Expr:

• It can be used to check whether dereference(cxxThisExpr()) is modified or not.
• For pointer types, it can be used to check both declRefExpr(isPointerType()) and dereference(declRefExpr(isPointerType())), and suggest adding const at different level

• match all relevant non-const variable declarations as potential candidates
• track all uses of the candidates and check for modification in their scope

One Note: We could include variables in unnamed namespaces and static declared variables. They have TU scope.

Great catch!

My deviations:

• a variable should be considered modified if a non-const handle is create from it, even if that handle does not modify its referenced value. As first step, those handles should be marked const, then the original value can be marked as const. That is required to produce compiling code after potential code-transformation.

I believe we can issue warning + fixit within one pass:

int f() {
  int a = 10;
  int& b = a;
  return b;
}

We should be able to issue warnings for both a and b, because b itself is a varDecl without modifying behavior, and when checking a we'll just repeat a bit more work that was already done when checking b by following the reference chain to be able to find that a can be const as well.

[JonasToth]

In D45444#1069488, @shuaiwang wrote:

I've updated https://reviews.llvm.org/D45679 and I think the isModified() function there is now sufficiently covering the cases you've covered here and can be used as a good starting version if you plan to use it here.
I copied your const-values test cases and it now passes with the following differences:

• All unused local variables removed, my check will issue a warning for them because technically they're not modified, but I understand why you don't want to cover them. I don't feel strongly about it and I removed it from my copy-pasted test cases because I just to demonstrate isModified()

Nice!

• Better recall in some_reference_taking, both np_local0 and r1_np_local0 can be caught, similar for np_local1 and non_const_ref in handle_from_array.

Yes. But i feel we need to gather more information. For potential checks, it might be nice to control to which scope modification is defined and/or emit notes for interesting events like taking a non-const handle which isnt modified, too.

It kinda feels like we are converging to a dependency graph for every variable that contains the information for dependencies and dependents + modification history. I dont think it is bad, but maybe complicated :)
What do you think about returning a ModificationRecord that contains the number of direct modification, the number of local non-const handles and escaping non-const handles.
Including const handles would be nice in the future for more and different analysis.

• direct_class_access np_local5 triggers with my check (shouldn't it?)

Yes can be const: Godbolt

• In range_for non-const loop variables triggers with my check (shouldn't they?)

• If the loop copies every value no modification occurs (its not a copy, its initialization. If the constructor would modify its value, the source cant be const either. Is it allowed?)
• if a handle is taken the usual rules apply.

• In range_for local arrays doesn't trigger with my check, because I'm treating ArrayToPointerDecay the same as taking address of a variable, and looping over an array would involve ArrayToPointerDecay when the implicit __begin/__end are initialized. I added a note inside isModified for future improvements.

Given this is implemented here, can you use it?

• My check over triggers for template (with my failed attempt to fix), but I think it's more of some mistake in the check itself rather than in isModified

I removed templates while matching for potential candidates. That can be moved out of the isModified(). isModified() can only reason about instantiated functions. For the actual template we need concepts.

Sorry about the confusion.
Basically consider this example:

class Foo {
public:
  void a() { x = 10; }
  void b() { // nothing }
  void c() { a(); }
  void d() { b(); }
  
private:
  int x;
};

If we check whether isModified(dereference(cxxThisExpr())) for each CompondStmt(hasParent(functionDecl())), we would conclude that:

• a() should stay non-const, because there's this->x = 10 modifying *this
• b() should be changed to const, because nothing modifies *this
• c() should stay non-const, because we're invoking non-const member function on this
• d() should also stay non-const, with the same reason for c(). (We can in theory be smarter about this one because the definition of b() happens to be inside the same TU but that's out of scope right now)

However if we use a per-TU map to record whether x can be const, we'll conclude that x is modified thus can't be const, missing the one that b() is not modifying x. To fix that we'll need two-layered map recording that x is only modified in a() and potentially modified indirectly from c() and d(), so that in the end we can figure out that b() is safe to be marked const.
Anyway, all I was trying to say is: let's use the isModified() approach as it's simpler & cleaner for future use cases like adding const to member functions. And it feels to me that we've already reached agreement there :)

Yes.

@shuaiwang Are you in IRC from time to time? I think it will be better to discuss changes in chat, when i start to introduce your approach here.

Not really, but I can get myself familiar with it.

We dont need to chat. The review discussion works well. :)

isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

What i understand from your comments and code:

• implement a utility taking DeclRefExpr and check if there is modification in some scope (Stmt, e.g. CompoundStmt) -> isModified or cantBeModifiedWithin

It doesn't have to be DeclRefExpr, any Expr should work, and it's useful to make it accept any Expr:

• It can be used to check whether dereference(cxxThisExpr()) is modified or not.
• For pointer types, it can be used to check both declRefExpr(isPointerType()) and dereference(declRefExpr(isPointerType())), and suggest adding const at different level

• match all relevant non-const variable declarations as potential candidates
• track all uses of the candidates and check for modification in their scope

One Note: We could include variables in unnamed namespaces and static declared variables. They have TU scope.

Great catch!

My deviations:

• a variable should be considered modified if a non-const handle is create from it, even if that handle does not modify its referenced value. As first step, those handles should be marked const, then the original value can be marked as const. That is required to produce compiling code after potential code-transformation.

I believe we can issue warning + fixit within one pass:

int f() {
  int a = 10;
  int& b = a;
  return b;
}

We should be able to issue warnings for both a and b, because b itself is a varDecl without modifying behavior, and when checking a we'll just repeat a bit more work that was already done when checking b by following the reference chain to be able to find that a can be const as well.

We can certainly try it. Speaking from previous experience: There is always some weird way to not catch everything and to get false positives in edge cases, thats why i tried the conservative way first. But i was stumbling around, too. I think the new function can give us more confidence, because its easier to test in isolation.

One interesting thing i found in code: int &some_var = condition ? var1 : var2. I am not sure if we already find that as reference taking :)

[JonasToth]

• [Feature] refactor check to use a utility function to determine constness

[JonasToth]

@shuaiwang I implemented the check on top of you utility function. It does fail right now (concentrating on values only for now). I added a FIXME for all false positives/negatives for the values test cases.

[JonasToth]

• [Misc] mark false positives

[JonasToth]

• [Feature] use new statefull const checker
• [Test] add more tests finding false positives

[shuaiwang]

clang-tidy/cppcoreguidelines/ConstCheck.cpp

229–237 ↗

(On Diff #144458)

I think we need to loop over usages instead of just checking the first, i.e.:

for (const auto &Nodes : Usage) {
  const auto* UseExpr = Nodes.getNodeAs<Expr>("use");
  if (UseExpr && isMutated(UseExpr)) return true;
}

I'll add a helper function in the MutationAnalyzer for checking varDecl directly as well per your comment there, which you'll be able to use directly in this check. Before that's ready (and if you have time of course) could you help check how many false positives are left with this change?

[JonasToth]

I will migrate to the new API today evening (european time).

Why do you think that looping is required? From my understanding, we
need the first usage (DeclRefExpr) to get the analysis started. The
analysis itself will check all remaining uses. Is this necessary,
because we analysis on a Expr basis?

Am 29.04.2018 um 20:19 schrieb Shuai Wang via Phabricator:

shuaiwang added inline comments.

Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:229-237
+ const auto *UseExpr = selectFirst<Expr>("use", Usage);
+
+ The declared variables was used in non-const conserving way and can not
+ be declared as const.
+ if (UseExpr && Scopes[Scope]->isMutated(UseExpr)) {
+ // diag(UseExpr->getLocStart(), "Investigating starting with this use",

+ // DiagnosticIDs::Note);

I think we need to loop over usages instead of just checking the first, i.e.:

for (const auto &Nodes : Usage) {
  const auto* UseExpr = Nodes.getNodeAs<Expr>("use");
  if (UseExpr && isMutated(UseExpr)) return true;
}

I'll add a helper function in the MutationAnalyzer for checking varDecl directly as well per your comment there, which you'll be able to use directly in this check. Before that's ready (and if you have time of course) could you help check how many false positives are left with this change?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

• migrate to Decl interface
• add template metaprogramming test

[JonasToth]

• fix bad template instantiation

[shuaiwang]

Why do you think that looping is required? From my understanding, we
need the first usage (DeclRefExpr) to get the analysis started. The
analysis itself will check all remaining uses. Is this necessary,
because we analysis on a Expr basis?

Yes. the analyzer traces starting from the given DeclRefExpr, e.g.:

int a; // <- varDecl
const int& b = a;  // <- first DeclRefExpr
int& c = a; // <- second DeclRefExpr

const int& b2 = b;
int& c2 = c;
c2 = 10;

If we start from the first DeclRefExpr, we'll only trace to b and then to b2. We need to start from varDecl (or loop over all DeclRefExpr) to be able to trace to the second DeclRefExpr from where we can get to c -> c2 -> c2 = 10.

[JonasToth]

I see. Thank you for fixing + explaining :)

Am 30.04.2018 um 23:39 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

Why do you think that looping is required? From my understanding, we
need the first usage (DeclRefExpr) to get the analysis started. The
analysis itself will check all remaining uses. Is this necessary,
because we analysis on a Expr basis?

Yes. the analyzer traces starting from the given DeclRefExpr, e.g.:

int a; // <- varDecl
const int& b = a;  // <- first DeclRefExpr
int& c = a; // <- second DeclRefExpr

const int& b2 = b;
int& c2 = c;
c2 = 10;

If we start from the first DeclRefExpr, we'll only trace to b and then to b2. We need to start from varDecl (or loop over all DeclRefExpr) to be able to trace to the second DeclRefExpr from where we can get to c -> c2 -> c2 = 10.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

• rebase on commited ExpMutationAnalyzer
• clean up, documentation

[JonasToth]

• [Misc] order in release notes

[JonasToth]

• fixed some comments

[JonasToth]

• Merge branch 'master' into check_const

[lebedev.ri]

Ping, i guess :)
Anything needed to get this going?

[JonasToth]

Time :/

But review comments are still appreciated :)

Am 14.07.2018 um 16:34 schrieb Roman Lebedev via Phabricator:

lebedev.ri added a comment.

Ping, i guess :)
Anything needed to get this going?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[0x8000-0000]

In D45444#1162715, @JonasToth wrote:

Time :/

Is this available as a public Git branch somewhere, or is downloading the diff the preferred way to interact with this code?

[JonasToth]

Yes.

https://github.com/JonasToth/clang-tools-extra/tree/check_const

This is the branch i work on. I got it up to date with the current
master for CTE. :)

Am 14.07.2018 um 20:59 schrieb Florin Iucha via Phabricator:

0x8000-0000 added a comment.

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

Time :/

Is this available as a public Git branch somewhere, or is downloading the diff the preferred way to interact with this code?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

@0x8000-0000 are you interested in working on the check? If i recall correctly the branch for 7.0 will happen on 1. august. That would be the timewindow in which it makes sense to give you the patch. I will have time in september following, but not right now.
I try to commit 2 bug fixes in this time, because they are almost done already.

@aaron.ballman What do you think is missing for this check? Do you think it is doable until 1. august?

[JonasToth]

Removed one unresolved comment, probably thats why it was stuck :D

[JonasToth]

• Merge branch 'master' into check_const
• [Misc] remove comment and iostream

[0x8000-0000]

In D45444#1164671, @JonasToth wrote:

@0x8000-0000 are you interested in working on the check? If i recall correctly the branch for 7.0 will happen on 1. august. That would be the timewindow in which it makes sense to give you the patch. I will have time in september following, but not right now.
I try to commit 2 bug fixes in this time, because they are almost done already.

@JonasToth
I am interested in helping - is there any area you'd like me to look at? I have reviewed the test cases and they seem fine. The code is a bit more intimidating, given that I just started looking at clang/llvm internals. But I have pulled the patch into my personal build at home and at ${BIG_CO} and I am using the check "in production". I have detected no crashes and no false positives yet.

[JonasToth]

What do you mean by "The code is a bit more intimidating", the check
itself or the amount of tests?

In general the utility piece that was commited before this check should
already analyze the constness good, given that this check builds upon it
the functionality should be ok.

In my opinion there is nothing major missing, but the reviewers (usually
@aaron.ballman) have to judge that. After that i would come back to you.

If you have a run on a codebase, you could attach logfile to see how
noisy the check is itself. :)

If you do have time, there are always bugs to fix :P

But you could also take a look at the enforcement of some coding
guidelines (my personal todolists:
https://github.com/JonasToth/CppCoreGuidelinesTooling 
https://github.com/JonasToth/HighIntegrityTooling). There are always low
hanging fruit checks. Implementing something there helps getting used to
the codebase of clang, too.

Am 17.07.2018 um 12:41 schrieb Florin Iucha via Phabricator:

@JonasToth
I am interested in helping - is there any area you'd like me to look at? I have reviewed the test cases and they seem fine. The code is a bit more intimidating, given that I just started looking at clang/llvm internals. But I have pulled the patch into my personal build at home and at ${BIG_CO} and I am using the check "in production". I have detected no crashes and no false positives yet.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[Eugene.Zelenko]

But you could also take a look at the enforcement of some coding
guidelines (my personal todolists:
https://github.com/JonasToth/CppCoreGuidelinesTooling 
https://github.com/JonasToth/HighIntegrityTooling). There are always low
hanging fruit checks. Implementing something there helps getting used to
the codebase of clang, too.

You may be also interested to look onto enhancements requests for Clang-tidy in http://bugs.llvm.org.

[0x8000-0000]

Any plans for fix-its that will add the suggested 'const' qualifiers?

[JonasToth]

Yes, but that is complicated. Functionality for that will first be
implemented in the libTooling and then utilized here later.

Am 19.07.2018 um 04:16 schrieb Florin Iucha via Phabricator:

0x8000-0000 added a comment.

Any plans for fix-its that will add the suggested 'const' qualifiers?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

@aaron.ballman Are you missing something in this check/tests? I think i will remove the big comment with my thoughts on what has to be done, or do you think it would help for reference?

I would probably be able to finalize this week if there is no major issue, so it could be committed for the release.

[JonasToth]

• Merge branch 'master' into check_const

[JonasToth]

get check up to 8.0

[aaron.ballman]

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

47

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

[JonasToth]

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

No problem :)

[JonasToth]

clang-tidy/cppcoreguidelines/ConstCheck.cpp

32 ↗

(On Diff #159062)

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

[xbolva00]

clang-tidy/cppcoreguidelines/ConstCheck.cpp

28 ↗

(On Diff #159062)

typo
paramters -> parameters

[JonasToth]

• Merge branch 'master' into check_const
• [Misc] rename and first review comments
• language stuff

[JonasToth]

clang-tidy/cppcoreguidelines/ConstCheck.cpp

141–142 ↗

(On Diff #159062)

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

[JonasToth]

• doc list

[JonasToth]

• 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]

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]

• fix typo

[JonasToth]

• update ReleaseNotes

[aaron.ballman]

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]

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]

clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp

184

Ups. The comment has them :D

[JonasToth]

• address review issues, todos/fixmes and diag nit

[JonasToth]

In D45444#1189262, @aaron.ballman wrote:

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 :(

[JonasToth]

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

const_correctness_2_llvm_lib_references1 MBDownload

const_correctness_2_llvm_lib_values_pointers18 MBDownload

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.

[shuaiwang]

• 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?

[JonasToth]

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]

• fix bug with AnalyzeValues==false skipping whole check, adjust test code to 'const' instead of const

[shuaiwang]

In D45444#1191874, @JonasToth wrote:

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.

[JonasToth]

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]

• explicitly ignore lambdas in VarDecls

[shuaiwang]

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

[JonasToth]

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

[JonasToth]

• get up to date

[JonasToth]

• ignore lambdas properly

[JonasToth]

llvm_lib_references.out486 KBDownload

llvm_lib_pointer_values.out9 MBDownload

llvm_lib_values.out12 MBDownload

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]

• update to ExprMutAnalyzer living in clang now

[xbolva00]

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

[JonasToth]

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

[JonasToth]

• fix actually use clang-analyses correctly

[xbolva00]

Are you still looking at this?

[JonasToth]

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]

• Merge branch 'master' into check_const

[JonasToth]

• Merge branch 'master' into check_const

[JonasToth]

• Merge branch 'master' into check_const

[JonasToth]

• avoid bitrot

[tsdgeos]

Would this warn with stuff like

double borderWidth;
[... code that doesn't use borderWidth ...]
borderWidth = border->getWidth(); 
[... code that reads borderWidth ...]

[JonasToth]

In D45444#1685995, @tsdgeos wrote:

Would this warn with stuff like

double borderWidth;
[... code that doesn't use borderWidth ...]
borderWidth = border->getWidth(); 
[... code that reads borderWidth ...]

Warn in what sense? That borderWidth could be initialized early and then be const?
No. It would only suggest const (if configured to make values const) when borderWidth is initialized and then untouched.

[JonasToth]

• update license
• rebase to master and patch for adding const

[0x8000-0000]

Thank you for rebasing on current master.

I have ran it today on our code base and found three classes of false positives:

1. Writing to a bitfield of a struct. The struct still is suggested it should be const.
2. Using a variable with an ostream extraction; like "int foo; cin >> foo;", except it was a template on the stream instance.
3. In a for loop, with a somewhat strange pair (for auto [foo, bar] = std::pair {}; foo < big_foo; ++ foo).

Let me know if you can't create test cases from these descriptions and I can try to extract the code.

[JonasToth]

In D45444#1766156, @0x8000-0000 wrote:

Thank you for rebasing on current master.

I have ran it today on our code base and found three classes of false positives:

1. Writing to a bitfield of a struct. The struct still is suggested it should be const.
2. Using a variable with an ostream extraction; like "int foo; cin >> foo;", except it was a template on the stream instance.
3. In a for loop, with a somewhat strange pair (for auto [foo, bar] = std::pair {}; foo < big_foo; ++ foo).

Let me know if you can't create test cases from these descriptions and I can try to extract the code.

Thank you for testing, I would appreciate if you test later versions, too!
I will focus on landing the utility function for adding const first. I noticed false positives in LLVM as well, I try to reproduce them all and fix/workaround the issues.

[0x8000-0000]

In D45444#1766852, @JonasToth wrote:

Thank you for testing, I would appreciate if you test later versions, too!
I will focus on landing the utility function for adding const first. I noticed false positives in LLVM as well, I try to reproduce them all and fix/workaround the issues.

I will test it as often as you rebase it :) Thank you for the useful tool!

Are you going to keep updating it on https://github.com/JonasToth/llvm-project/commits/feature_check_const ?

[JonasToth]

Are you going to keep updating it on https://github.com/JonasToth/llvm-project/commits/feature_check_const ?

Yup, thats my backup if something goes wrong and to synchronize my pcs. :)

[JonasToth]

Abonding this revision in favor of: https://reviews.llvm.org/D54943
It doesn't make sense to have both open at the same time, as the const-transformation happens in its own patch and no heavy lifting is done here.

clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   AvoidGotoCheck.cpp
+  ConstCorrectnessCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
   MacroUsageCheck.cpp
   NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp
   OwningMemoryCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsConstantArrayIndexCheck.cpp

clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h

+//===--- ConstCorrectnessCheck.h - clang-tidy--------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCORRECTNESSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCORRECTNESSCHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
+
+namespace clang {
+namespace tidy {
+
+namespace cppcoreguidelines {
+
+/// This check warns on variables which could be declared const but are not.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-const.html
+class ConstCorrectnessCheck : public ClangTidyCheck {
+public:
+  ConstCorrectnessCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        AnalyzeValues(Options.get("AnalyzeValues", 1)),
+        AnalyzeReferences(Options.get("AnalyzeReferences", 1)),
+        WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)) {}
+
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void registerScope(const CompoundStmt *LocalScope, ASTContext *Context);
+
+  using MutationAnalyzer = std::unique_ptr<ExprMutationAnalyzer>;
+  llvm::DenseMap<const CompoundStmt *, MutationAnalyzer> ScopesCache;
+
+  const bool AnalyzeValues;
+  const bool AnalyzeReferences;
+  const bool WarnPointersAsValues;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCORRECTNESSCHECK_H

clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp

+//===--- ConstCorrectnessCheck.cpp - clang-tidy----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ConstCorrectnessCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/*
+ * NOTE: This massive comment will be removed before committing. It serves as
+ * notebook on what to keep an eye on for now.
+ *
+ * General Thoughts
+ * ================
+ *
+ * For now: Only local variables are considered. Globals/namespace variables,
+ * parameters and class members are not analyzed.
+ * Parameters have a check already: readability-non-const-parameter
+ *
+ *
+ * TODO Add support for Obj-C object pointers.
+ * Handle = either a pointer or reference
+ * Value  = everything else (Type variable_name;)
+ *
+ * Value Semantic
+ * ==============
+ *  - it is neither global nor namespace level                        + CHECK
+ *  - it never gets assigned to after initialization                  + CHECK
+ *    -> every uninitialized variable can not be const                + CHECK
+ *  - no non-const handle is created with it                          + CHECK
+ *    - no non-const pointer from it                                  + CHECK
+ *    - no non-const pointer argument                                 + CHECK
+ *    - no non-const reference from it                                + CHECK
+ *    - no non-const reference argument                               + CHECK
+ *    - no non-const capture by reference in a lambda                 + CHECK
+ *  - it is not returned as non-const handle from a function          + CHECK
+ *  - it address is not assigned to an out pointer parameter          + CHECK
+ *  - Lambdas follow value semantics, but should be ignored           + CHECK
+ *
+ * primitive Builtins
+ * ----------------
+ *  - it is not modified with an operator (++i,i++,--i,i--)           + CHECK
+ *  - it is not modified with an operator-assignment                  + CHECK
+ *
+ * objects
+ * -------
+ *  - there is no non-const access to a member
+ *  - there is no call to a non-const method                          + CHECK
+ *  - there is no call to an non-const overloaded operator            + CHECK
+ *  - there is no non-const iterator created from this type           + CHECK
+ *    (std::begin and friends)
+ *
+ * arrays
+ * ------
+ *  - there is no non-const operator[] access                         + CHECK
+ *  - there is no non-const handle creation of one of the elements    + CHECK
+ *  - there is no non-const iterator created from this type           + CHECK
+ *    (std::begin and friends)
+ *
+ * templated variables
+ * -------------------
+ *  - one can not reason about templated variables, because every sensible
+ *    operation is overloadable and different instantiations will result
+ *    in types with different const-properties.
+ *  - Example: operator+(T& lhs, T& rhs) -> modification might occur for this
+ * type
+ *    -> this fordbids `val = val1 + val2` val1 and val2 to be const
+ *  - only concepts give possibility to infer constness of templated variables
+ *
+ * Handle Semantic
+ * ===============
+ *  - modification of the pointee prohibit constness
+ *  - Handles follow the typ of the pointee
+ *
+ *  - no assignment to the target of the handle
+ *
+ * pointers
+ * --------
+ *  - match both for value and handle semantic
+ *
+ * references
+ * ----------
+ *  - only handle semantic applies
+ *  - references to templated types suffer from the same problems as templated
+ *    values
+ *
+ * forwarding reference
+ * --------------------
+ *  - same as references?
+ *
+ * Implementation strategy
+ * =======================
+ *
+ *  - Register every declared local variable/constant with value semantic.
+ *    (pointers, values)
+ *    Store if they can be made const.
+ *    (const int i = 10 : no,
+ *     int *const = &i  : no,
+ *     int i = 10       : yes, -> const int i = 10
+ *     int *p_i = &i    : yes, -> int *const p_i = &i)
+ *  - Register every declared local variable/constant with handle semantic.
+ *    (pointers, references)
+ *    Store if they can be made const, meaning if they can be a const target
+ *    (const int *cp_i = &i : no,
+ *     const int &cr_i = i  : no,
+ *     int *p_i = &i        : yes, -> const int *p_i = &i
+ *     int &r_i = i         : yes, -> const int &r_i = i)
+ *  - Keep 2 dictionaries for values and handles
+ *
+ *  - Match operations/events that forbid values to be const -> mark then 'no'
+ *  - Match operations/events that forbid handles to be const -> mark then 'no'
+ *
+ *  - once the translation unit is finished, determine what can be const, by
+ *    just iterating over all keys and check if they map to 'true'.
+ *    - values that can be const -> emit warning for their type and name
+ *    - handles that can be const -> emit warning for the pointee type and name
+ *    - ignore the rest
+ */
+
+namespace {
+AST_MATCHER(VarDecl, isLocal) { return Node.isLocalVarDecl(); }
+} // namespace
+
+void ConstCorrectnessCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AnalyzeValues", AnalyzeValues);
+  Options.store(Opts, "AnalyzeReferences", AnalyzeReferences);
+  Options.store(Opts, "WarnPointersAsValues", WarnPointersAsValues);
+}
+
+void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ConstType = hasType(isConstQualified());
+  const auto ConstReference = hasType(references(isConstQualified()));
+  const auto TemplateType = anyOf(hasType(templateTypeParmType()),
+                                  hasType(substTemplateTypeParmType()));
+
+  // FIXME Investigate the DeMorgan-simplification for the logical expression.
+  // Match local variables which could be const.
+  // Example: `int i = 10`, `int i` (will be used if program is correct)
+  const auto LocalValDecl = varDecl(allOf(
+      isLocal(), hasInitializer(anything()),
+      unless(hasType(cxxRecordDecl(isLambda()))), unless(ConstType),
+      unless(ConstReference), unless(TemplateType), unless(isImplicit())));
+
+  // Match the function scope for which the analysis of all local variables
+  // shall be run.
+  const auto FunctionScope =
+      functionDecl(allOf(hasBody(compoundStmt().bind("scope")),
+                         findAll(LocalValDecl.bind("new-local-value"))));
+  Finder->addMatcher(FunctionScope, this);
+}
+
+void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *LocalScope = Result.Nodes.getNodeAs<CompoundStmt>("scope");
+  assert(LocalScope && "Did not match scope for local variable");
+  registerScope(LocalScope, Result.Context);
+
+  const auto *Variable = Result.Nodes.getNodeAs<VarDecl>("new-local-value");
+  assert(Variable && "Did not match local variable definition");
+
+  // Each variable can only in one category: Value, Pointer, Reference.
+  // Analysis can be controlled for every category.
+  if (Variable->getType()->isReferenceType() && !AnalyzeReferences)
+    return;
+
+  if (Variable->getType()->isPointerType() && !WarnPointersAsValues)
+    return;
+
+  if (!(Variable->getType()->isReferenceType() ||
+        Variable->getType()->isPointerType()) &&
+      !AnalyzeValues)
+    return;
+
+  // Offload const-analysis to utility function.
+  if (ScopesCache[LocalScope]->isMutated(Variable))
+    return;
+
+  // TODO Implement automatic code transformation to add the 'const'.
+  diag(Variable->getBeginLoc(),
+       "variable %0 of type %1 can be declared 'const'")
+      << Variable << Variable->getType();
+}
+
+void ConstCorrectnessCheck::registerScope(const CompoundStmt *LocalScope,
+                                          ASTContext *Context) {
+  if (ScopesCache.find(LocalScope) == ScopesCache.end()) {
+    ScopesCache.insert(std::make_pair(
+        LocalScope,
+        llvm::make_unique<ExprMutationAnalyzer>(*LocalScope, *Context)));
+  }
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.cpp - clang-tidy -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidGotoCheck.h"
+#include "ConstCorrectnessCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "MacroUsageCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"

 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
+    CheckFactories.registerCheck<ConstCorrectnessCheck>(
+        "cppcoreguidelines-const-correctness");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<MacroUsageCheck>(
         "cppcoreguidelines-macro-usage");
     CheckFactories.registerCheck<NarrowingConversionsCheck>(
         "cppcoreguidelines-narrowing-conversions");
     CheckFactories.registerCheck<NoMallocCheck>("cppcoreguidelines-no-malloc");
     CheckFactories.registerCheck<misc::NonPrivateMemberVariablesInClassesCheck>(

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 
   Checks for uses of ``absl::Duration`` division that is done in a
   floating-point context, and recommends the use of a function that
   returns a floating-point value.
 
 - New :doc:`abseil-duration-factory-float
   <clang-tidy/checks/abseil-duration-factory-float>` check.
 
   Checks for cases where the floating-point overloads of various
   ``absl::Duration`` factory functions are called when the more-efficient
- New :doc:`abseil-redundant-strcat-calls
   being passed to another ``absl::StrCat`` or ``absl::StrAppend``.
 
 - New :doc:`abseil-str-cat-append
   <clang-tidy/checks/abseil-str-cat-append>` check.
 
   Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests
   ``absl::StrAppend()`` should be used instead.
 
+- New :doc:`cppcoreguidelines-const-correctness
+  <clang-tidy/checks/cppcoreguidelines-const-correctness>` check.
+
+  Suggest adding ``const`` to unmodified local variables.
+
 - New :doc:`cppcoreguidelines-macro-usage
   <clang-tidy/checks/cppcoreguidelines-macro-usage>` check.
 
   Finds macro usage that is considered problematic because better language
   constructs exist for the task.
 
 - New :doc:`misc-non-private-member-variables-in-classes
   <clang-tidy/checks/misc-non-private-member-variables-in-classes>` check.
- New :doc:`readability-isolate-decl
   Detects local variable declarations declaring more than one variable and
   tries to refactor the code to one statement per declaration.
 
 - New :doc:`readability-const-return-type
   <clang-tidy/checks/readability-const-return-type>` check.
 
   Checks for functions with a ``const``-qualified return type and recommends
   removal of the ``const`` keyword.
+>>>>>>> master
 
 - New :doc:`readability-magic-numbers
   <clang-tidy/checks/readability-magic-numbers>` check.
 
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
 - New :doc:`readability-uppercase-literal-suffix

docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst

+.. title:: clang-tidy - cppcoreguidelines-const-correctness
+
+cppcoreguidelines-const-correctness
+===================================
+
+This check implements detection of local variables which could be declared as
+``const``, but are not. Declaring variables as ``const`` is required by many
+coding guidelines, such as:
+`CppCoreGuidelines ES.25 <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es25-declare-an-object-const-or-constexpr-unless-you-want-to-modify-its-value-later-on>`_
+and `High Integrity C++ 7.1.2 <http://www.codingstandard.com/rule/7-1-2-use-const-whenever-possible/>`_.
+
+Please note that this analysis is type-based only. Variables that are not modified
+but non-const handles might escape out of the scope are not diagnosed as potential
+``const``.
+
+.. code-block:: c++
+  
+  // Declare a variable, which is not ``const`` ...
+  int i = 42;
+  // but use it as read-only. This means that `i` can be declared ``const``.
+  int result = i * i;
+
+The check analyzes values, pointers and references (if configured that way).
+For better understanding some code samples:
+
+.. code-block:: c++
+
+  // Normal values like built-ins or objects.
+  int potential_const_int = 42;
+  int copy_of_value = potential_const_int;
+
+  MyClass could_be_const;
+  could_be_const.const_qualified_method();
+
+  // References can be declared const as well.
+  int &reference_value = potential_const_int;
+  int another_copy = reference_value;
+
+  // Similar behaviour for pointers.
+  int *pointer_variable = &potential_const_int;
+  int last_copy = *pointer_variable;
+
+
+Options
+-------
+
+.. option:: AnalyzeValues (default = 1)
+
+  Enable or disable the analysis of ordinary value variables, like ``int i = 42;``
+
+.. option:: AnalyzeReferences (default = 1)
+
+  Enable or disable the analysis of reference variables, like ``int &ref = i;``
+
+.. option:: WarnPointersAsValues (default = 0)
+
+  This option enables the suggestion for ``const`` of the pointer itself.
+  Pointer values have two possibilities to be ``const``, the pointer itself
+  and the value pointing to. 
+
+  .. code-block:: c++
+
+    const int value = 42;
+    const int * const pointer_variable = &value;
+    
+    // The following operations are forbidden for `pointer_variable`.
+    // *pointer_variable = 44;
+    // pointer_variable = nullptr;

docs/clang-tidy/checks/list.rst

.. toctree::
    cert-msc30-c (redirects to cert-msc50-cpp) <cert-msc30-c>
    cert-msc32-c (redirects to cert-msc51-cpp) <cert-msc32-c>
    cert-msc50-cpp
    cert-msc51-cpp
    cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
    cppcoreguidelines-avoid-goto
    cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) <cppcoreguidelines-avoid-magic-numbers>
    cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) <cppcoreguidelines-c-copy-assignment-signature>
+   cppcoreguidelines-const-correctness
    cppcoreguidelines-interfaces-global-init
    cppcoreguidelines-macro-usage
    cppcoreguidelines-narrowing-conversions
    cppcoreguidelines-no-malloc
    cppcoreguidelines-non-private-member-variables-in-classes (redirects to misc-non-private-member-variables-in-classes) <cppcoreguidelines-non-private-member-variables-in-classes>
    cppcoreguidelines-owning-memory
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index

test/clang-tidy/cppcoreguidelines-const-correctness-pointer-as-values.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "cppcoreguidelines-const-correctness.AnalyzeValues", value: 1},\
+// RUN:   {key: "cppcoreguidelines-const-correctness.WarnPointersAsValues", value: 1}]}' \
+// RUN: --
+
+void potential_const_pointer() {
+  double np_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  double *p_local0 = &np_local0[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
+}

test/clang-tidy/cppcoreguidelines-const-correctness-values.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t
+
+// ------- Provide test samples for primitive builtins ---------
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+    int p_local1 = 42;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+    np_local0 *= 2;
+  }
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = &np_local0;
+  int *const p1_np_local0 = &np_local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = &np_local1;
+  int *const p1_np_local1 = &np_local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(&np_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = &np_local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  const int *const p0_p_local1 = &p_local1;
+
+  int p_local2 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared 'const'
+  function_in_pointer(&p_local2);
+}
+
+void function_inout_ref(int &inout);
+void function_in_ref(const int &in);
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int &r0_np_local0 = np_local0;
+  int &r1_np_local0 = np_local0;
+  r1_np_local0 = 43;
+  const int &r2_np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  const int &r0_p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
+  double np_local0 = 24.4;
+
+  return &np_local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared 'const'
+  return &p_local1;
+}
+
+double &non_const_ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
+  double np_local0 = 42.42;
+  return np_local0;
+}
+
+const double &const_ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared 'const'
+  return p_local1;
+}
+
+double *&return_non_const_pointer_ref() {
+  double *np_local0 = nullptr;
+  return np_local0;
+}
+
+void overloaded_arguments(const int &in);
+void overloaded_arguments(int &inout);
+void overloaded_arguments(const int *in);
+void overloaded_arguments(int *inout);
+
+void function_calling() {
+  int np_local0 = 42;
+  overloaded_arguments(np_local0);
+
+  const int np_local1 = 42;
+  overloaded_arguments(np_local1);
+
+  int np_local2 = 42;
+  overloaded_arguments(&np_local2);
+
+  const int np_local3 = 42;
+  overloaded_arguments(&np_local3);
+}
+
+template <typename T>
+void define_locals(T np_arg0, T &np_arg1, int np_arg2) {
+  T np_local0 = 0;
+  np_local0 += np_arg0 * np_arg1;
+
+  T np_local1 = 42;
+  np_local0 += np_local1;
+
+  // Used as argument to an overloaded function with const and non-const.
+  T np_local2 = 42;
+  overloaded_arguments(np_local2);
+
+  int np_local4 = 42;
+  // non-template values are ok still.
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  np_local4 += p_local0;
+}
+
+void template_instantiation() {
+  const int np_local0 = 42;
+  int np_local1 = 42;
+
+  define_locals(np_local0, np_local1, np_local0);
+  define_locals(np_local1, np_local1, np_local1);
+}
+
+struct ConstNonConstClass {
+  ConstNonConstClass();
+  ConstNonConstClass(double &np_local0);
+  double nonConstMethod() {}
+  double constMethod() const {}
+  double modifyingMethod(double &np_arg0) const;
+
+  double NonConstMember;
+  const double ConstMember;
+
+  double &NonConstMemberRef;
+  const double &ConstMemberRef;
+
+  double *NonConstMemberPtr;
+  const double *ConstMemberPtr;
+};
+
+void direct_class_access() {
+  ConstNonConstClass np_local0;
+
+  np_local0.constMethod();
+  np_local0.nonConstMethod();
+
+  ConstNonConstClass p_local0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'ConstNonConstClass' can be declared 'const'
+  p_local0.constMethod();
+
+  ConstNonConstClass p_local1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'ConstNonConstClass' can be declared 'const'
+  double np_local1;
+  p_local1.modifyingMethod(np_local1);
+
+  double np_local2;
+  ConstNonConstClass p_local2(np_local2);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'ConstNonConstClass' can be declared 'const'
+
+  ConstNonConstClass np_local3;
+  np_local3.NonConstMember = 42.;
+
+  ConstNonConstClass np_local4;
+  np_local4.NonConstMemberRef = 42.;
+
+  ConstNonConstClass p_local3;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'ConstNonConstClass' can be declared 'const'
+  const double val0 = p_local3.NonConstMember;
+  const double val1 = p_local3.NonConstMemberRef;
+  const double val2 = *p_local3.NonConstMemberPtr;
+
+  ConstNonConstClass p_local4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local4' of type 'ConstNonConstClass' can be declared 'const'
+  *np_local4.NonConstMemberPtr = 42.;
+}
+
+void class_access_array() {
+  ConstNonConstClass np_local0[2];
+  np_local0[0].constMethod();
+  np_local0[1].constMethod();
+  np_local0[1].nonConstMethod();
+
+  ConstNonConstClass p_local0[2];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'ConstNonConstClass [2]' can be declared 'const'
+  p_local0[0].constMethod();
+  np_local0[1].constMethod();
+}
+
+struct OperatorsAsConstAsPossible {
+  OperatorsAsConstAsPossible &operator+=(const OperatorsAsConstAsPossible &rhs);
+  OperatorsAsConstAsPossible operator+(const OperatorsAsConstAsPossible &rhs) const;
+};
+
+struct NonConstOperators {
+};
+NonConstOperators operator+(NonConstOperators &lhs, NonConstOperators &rhs);
+NonConstOperators operator-(NonConstOperators lhs, NonConstOperators rhs);
+
+void internal_operator_calls() {
+  OperatorsAsConstAsPossible np_local0;
+  OperatorsAsConstAsPossible np_local1;
+  OperatorsAsConstAsPossible p_local0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'OperatorsAsConstAsPossible' can be declared 'const'
+  OperatorsAsConstAsPossible p_local1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'OperatorsAsConstAsPossible' can be declared 'const'
+
+  np_local0 += p_local0;
+  np_local1 = p_local0 + p_local1;
+
+  NonConstOperators np_local2;
+  NonConstOperators np_local3;
+  NonConstOperators np_local4;
+
+  np_local2 = np_local3 + np_local4;
+
+  NonConstOperators p_local2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'NonConstOperators' can be declared 'const'
+  NonConstOperators p_local3 = p_local2 - p_local2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'NonConstOperators' can be declared 'const'
+}
+
+struct MyVector {
+  double *begin();
+  const double *begin() const;
+
+  double *end();
+  const double *end() const;
+
+  double &operator[](int index);
+  double operator[](int index) const;
+
+  double values[100];
+};
+
+void vector_usage() {
+  double np_local0[10];
+  np_local0[5] = 42.;
+
+  MyVector np_local1;
+  np_local1[5] = 42.;
+
+  double p_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double [10]' can be declared 'const'
+  double p_local1 = p_local0[5];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared 'const'
+
+  // The following subscript calls suprisingly choose the non-const operator
+  // version.
+  MyVector np_local2;
+  double p_local2 = np_local2[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'double' can be declared 'const'
+
+  MyVector np_local3;
+  const double np_local4 = np_local3[42];
+
+  // This subscript results in const overloaded operator.
+  const MyVector np_local5{};
+  double p_local3 = np_local5[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'double' can be declared 'const'
+}
+
+void const_handle(const double &np_local0);
+void const_handle(const double *np_local0);
+
+void non_const_handle(double &np_local0);
+void non_const_handle(double *np_local0);
+
+void handle_from_array() {
+  // Non-const handle from non-const array forbids declaring the array as const
+  double np_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  double *p_local0 = &np_local0[1]; // Could be `double *const`, but warning deactivated by default
+
+  double np_local1[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  double &non_const_ref = np_local1[1];
+  non_const_ref = 42.;
+
+  double np_local2[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  double *np_local3;
+  np_local3 = &np_local2[5];
+
+  double np_local4[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  non_const_handle(np_local4[2]);
+  double np_local5[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  non_const_handle(&np_local5[2]);
+
+  // Constant handles are ok
+  double p_local1[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double [10]' can be declared 'const'
+  const double *p_local2 = &p_local1[2]; // Could be `const double *const`, but warning deactivated by default
+
+  double p_local3[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'double [10]' can be declared 'const'
+  const double &const_ref = p_local3[2];
+
+  double p_local4[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local4' of type 'double [10]' can be declared 'const'
+  const double *const_ptr;
+  const_ptr = &p_local4[2];
+
+  double p_local5[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local5' of type 'double [10]' can be declared 'const'
+  const_handle(p_local5[2]);
+  double p_local6[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local6' of type 'double [10]' can be declared 'const'
+  const_handle(&p_local6[2]);
+}
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  for (int &non_const_ref : np_local0) {
+    non_const_ref = 42;
+  }
+
+  int np_local1[2] = {1, 2};
+  for (auto &non_const_ref : np_local1) {
+    non_const_ref = 43;
+  }
+
+  int np_local2[2] = {1, 2};
+  for (auto &&non_const_ref : np_local2) {
+    non_const_ref = 44;
+  }
+
+  // FIXME the warning message is suboptimal. It could be defined as
+  // `int *const np_local3[2]` because the pointers are not reseated.
+  // But this is not easily deducable from the warning.
+  int *np_local3[2] = {&np_local0[0], &np_local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local3' of type 'int *[2]' can be declared 'const'
+  for (int *non_const_ptr : np_local3) {
+    *non_const_ptr = 45;
+  }
+
+  // FIXME same as above, but silenced
+  int *const np_local4[2] = {&np_local0[0], &np_local0[1]};
+  for (auto *non_const_ptr : np_local4) {
+    *non_const_ptr = 46;
+  }
+
+  int p_local0[2] = {1, 2};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int [2]' can be declared 'const'
+  for (int value : p_local0) {
+    // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'value' of type 'int' can be declared 'const'
+  }
+
+  int p_local1[2] = {1, 2};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int [2]' can be declared 'const'
+  for (const int &const_ref : p_local1) {
+  }
+
+  int *p_local2[2] = {&np_local0[0], &np_local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
+  for (const int *con_ptr : p_local2) {
+  }
+
+  int *p_local3[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const'
+  for (const auto *con_ptr : p_local3) {
+  }
+}
+
+inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
+
+struct Value {
+};
+void placement_new() {
+  Value Mem;
+  Value *V = new (&Mem) Value;
+}
+
+struct ModifyingConversion {
+  operator int() { return 15; }
+};
+struct NonModifyingConversion {
+  operator int() const { return 15; }
+};
+void conversion_operators() {
+  ModifyingConversion np_local0;
+  NonModifyingConversion p_local0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'NonModifyingConversion' can be declared 'const'
+
+  int np_local1 = np_local0;
+  np_local1 = p_local0;
+}
+
+void casts() {
+  decltype(sizeof(void *)) p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'decltype(sizeof(void *))' (aka 'unsigned long') can be declared 'const'
+  auto np_local0 = reinterpret_cast<void *>(p_local0);
+  np_local0 = nullptr;
+
+  int p_local1 = 43;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  short p_local2 = static_cast<short>(p_local1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'short' can be declared 'const'
+
+  int np_local1 = p_local2;
+  int &np_local2 = static_cast<int &>(np_local1);
+  np_local2 = 5;
+}
+
+void ternary_operator() {
+  int np_local0 = 1, np_local1 = 2;
+  int &np_local2 = true ? np_local0 : np_local1;
+  np_local2 = 2;
+
+  int p_local0 = 3, np_local3 = 5;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  const int &np_local4 = true ? p_local0 : ++np_local3;
+
+  int np_local5[3] = {1, 2, 3};
+  int &np_local6 = np_local5[1] < np_local5[2] ? np_local5[0] : np_local5[2];
+  np_local6 = 42;
+
+  int np_local7[3] = {1, 2, 3};
+  int *np_local8 = np_local7[1] < np_local7[2] ? &np_local7[0] : &np_local7[2];
+  *np_local8 = 42;
+}
+
+// taken from http://www.cplusplus.com/reference/type_traits/integral_constant/
+template <typename T, T v>
+struct integral_constant {
+  static constexpr T value = v;
+  using value_type = T;
+  using type = integral_constant<T, v>;
+  constexpr operator T() { return v; }
+};
+
+template <typename T>
+struct is_integral : integral_constant<bool, false> {};
+template <>
+struct is_integral<int> : integral_constant<bool, true> {};
+
+template <typename T>
+struct not_integral : integral_constant<bool, false> {};
+template <>
+struct not_integral<double> : integral_constant<bool, true> {};
+
+// taken from http://www.cplusplus.com/reference/type_traits/enable_if/
+template <bool Cond, typename T = void>
+struct enable_if {};
+
+template <typename T>
+struct enable_if<true, T> { using type = T; };
+
+template <typename T>
+struct TMPClass {
+  T alwaysConst() const { return T{}; }
+
+  template <typename T2 = T, typename = typename enable_if<is_integral<T2>::value>::type>
+  T sometimesConst() const { return T{}; }
+
+  template <typename T2 = T, typename = typename enable_if<not_integral<T2>::value>::type>
+  T sometimesConst() { return T{}; }
+};
+
+void meta_type() {
+  TMPClass<int> p_local0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'TMPClass<int>' can be declared 'const'
+  p_local0.alwaysConst();
+  p_local0.sometimesConst();
+
+  TMPClass<double> p_local1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'TMPClass<double>' can be declared 'const'
+  p_local1.alwaysConst();
+
+  TMPClass<double> np_local0;
+  np_local0.alwaysConst();
+  np_local0.sometimesConst();
+}