This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
ClosedPublic

Authored by shuaiwang on Apr 15 2018, 6:20 PM.

Details

Summary

(Originally started as a clang-tidy check but there's already D45444 so shifted to just adding ExprMutationAnalyzer)

ExprMutationAnalyzer is a generally useful helper that can be used in different clang-tidy checks for checking whether a given expression is (potentially) mutated within a statement (typically the enclosing compound statement.)

This is a more general and more powerful/accurate version of isOnlyUsedAsConst, which is used in ForRangeCopyCheck, UnnecessaryCopyInitialization.

It should also be possible to construct checks like D45444 (suggest adding const to variable declaration) or https://bugs.llvm.org/show_bug.cgi?id=21981 (suggest adding const to member function) using this helper function.

This function is tested by itself and is intended to stay generally useful instead of tied to any particular check.

Diff Detail

Event Timeline

shuaiwang created this revision.Apr 15 2018, 6:20 PM
Eugene.Zelenko retitled this revision from Add a new check, readability-unmodified-non-const-variable, that finds declarations of non-const variables that never get modified. to [clang-tidy] Add a new check, readability-unmodified-non-const-variable, that finds declarations of non-const variables that never get modified..Apr 15 2018, 7:21 PM
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added a subscriber: JonasToth.

Hi @shuaiwang, i am currently working on the same check and we should definilty coordinate our work. :)

What do you think about joining the forces and to work together on such a check?

Things i would like to mention:

  • i differentiate between values and handles. I want to be able to transform to const int * const if possible, similar for references
  • i believe you do not test on all occasion, a variable can not be const, e.g. if it is casted, if it is catched by a lambda, array access, access through member functions and object members

Did you run the check over a codebase like llvm yet and what did you find?

JonasToth added inline comments.Apr 16 2018, 4:34 AM
clang-tidy/utils/ASTUtils.cpp
85 ↗(On Diff #142588)

there is a util::isAssignmentOperator in clang-tidy already.

shuaiwang updated this revision to Diff 142730.Apr 16 2018, 7:21 PM

Updated mostly isModified().
I'd like to mostly demonstrate that isModified() works given that there's https://reviews.llvm.org/D45444 and we'd like to merge.

Change to just add a helper function isModified

shuaiwang retitled this revision from [clang-tidy] Add a new check, readability-unmodified-non-const-variable, that finds declarations of non-const variables that never get modified. to [clang-tidy] Add a helper function isModified, that checks whether an expression is modified within a statement..Apr 16 2018, 11:15 PM
shuaiwang edited the summary of this revision. (Show Details)
shuaiwang marked an inline comment as done.Apr 16 2018, 11:19 PM

Hi @alexfh, @hokein, @JonasToth, I've updated this diff to be just adding the helper function isModified() with a set of dedicated unit tests. What do you think of the approach of getting this change committed and then @JonasToth can continue building D45444 by using isModified()?

Short note here too: I think having isModified return a track record, like a vector for each

  1. modifications
  2. non-const handle taking
  3. (const usages)

would be nice. Every user can decide how to react to it. Then the function would be more like usageRecord with pointers to each usage.
That allows graph building, it allows detailed diagnostics and contains all relevant information for the expr.

Other interesting cases we need to consider here:

  1. casts -> all kinds of casts forbid kinda forbid constness.
  2. rvalue references, modification happens for std::move(v); std::forward(v); static_cast<T&&>(v)
  3. ternary operator
  4. lambdas with local usage only
  5. lambdas that escape, e.g. int i; std::thread t([&](){ while(true) i++; }); Hard to analyze? I think we can consider all references into lambdas as escape

Maybe you can add an assertion about template types without concepts as a safeguard?

*hust* /llvm/tools/clang/lib/Analysis/PseudoConstantAnalysis.cpp

I will check this one first, before we get crazy and implemented something twice.

*hust* /llvm/tools/clang/lib/Analysis/PseudoConstantAnalysis.cpp

I will check this one first, before we get crazy and implemented something twice.

But this is IR-level analysis. Clang has own include/clang/Analysis.

*hust* /llvm/tools/clang/lib/Analysis/PseudoConstantAnalysis.cpp

I will check this one first, before we get crazy and implemented something twice.

But this is IR-level analysis. Clang has own include/clang/Analysis.

I investigated a bit and it seems to be deadcode :(
It was once used by IdempotentOperationChecker which got deleted 4 years ago: http://llvm.org/viewvc/llvm-project?view=revision&revision=198476
I'll take a look and see if I can learn something from it anyway.

shuaiwang updated this revision to Diff 142870.Apr 17 2018, 5:29 PM
shuaiwang edited the summary of this revision. (Show Details)

Handle casts, ternary operator and lambdas.
Also optionally returns a chain of Stmt giving more details about how the conclusion is reached.

shuaiwang updated this revision to Diff 142878.Apr 17 2018, 7:17 PM

Handle range-based for loop

Better range for loop handling.

You are doing a great job and i learn new stuff :)

Inspired by the analysis tool in clangs repo:

  • What do you think about having these functions in a class? Now, we need to recalculate and reanalyze the scope for every variable, multiple times (reference tracking). It would be nice to do it as lazy as possible and memorize the results. Especially addressing the use-case for the const-check, storing that a reference is not modified will save a lot of work = performance
  • Do we need to distinguish between Espaced and Modified? Having only two states will simplify some calculations (e.g. if (std::none_of(Expr, isModified) return EMK_Const)
  • i think the multiple analysis sections could be functions on their own. If you create a class for the check, these should be private methods or helpers. At the moment, some sections are hard to understand and some simplifications could be made.
  • what do you think creating a real ModificationReport that is stored per Expr? That can be helpful for a check like readability-complex-modification, dependency analysis and others. The Chain is in that direction, but not consistent from what i see. Storing this chain in the potential ModificationAnalyzer class would be superb.
clang-tidy/utils/ASTUtils.cpp
125 ↗(On Diff #142882)

I am suprised that callExpr does not cover constructor calls. Or is there more?

149 ↗(On Diff #142882)

Having the history for the trivial modifications would be nice, too.

I think treating all kinds of modifications is best.

160 ↗(On Diff #142882)

Having such a lambda is somewhat weird and redundant, because it mimics the original function.

I think that should be refactored with a short discussion in the general comments if thats ok for you.

180 ↗(On Diff #142882)

The implicit NotModified == 0 is hard to see.
Maybe a transition towards a std::any_of(Expr, isModified) is more readable. (see general comment)

188 ↗(On Diff #142882)

Same + code duplication.

200 ↗(On Diff #142882)

dito

220 ↗(On Diff #142882)

This section of the code is hard to understand.

222 ↗(On Diff #142882)

The pop is not clear to me

246 ↗(On Diff #142882)

Code duplication for finding all declRefExpr to that expr.

252 ↗(On Diff #142882)

same + code duplication

unittests/clang-tidy/IsModifiedTest.cpp
138 ↗(On Diff #142882)

Could you please add tests for overloaded operators as free functions?

Arithmetic operators can usually be free functions and take by const& or &.

You are doing a great job and i learn new stuff :)

  • What do you think about having these functions in a class? Now, we need to recalculate and reanalyze the scope for every variable, multiple times (reference tracking). It would be nice to do it as lazy as possible and memorize the results. Especially addressing the use-case for the const-check, storing that a reference is not modified will save a lot of work = performance

It may be reasonable to have variables/data members dependencies graph and mark them as constant/non constant.

I was thinking about that too.

  • We can extract all DeclRefExpr for the modifiying expressions and

add dependencies.

Such analysis is definitly interesting, but should maybe be added later.
Having an internal Expr : Modified? mapping would suffice, too. The
isExprModified will then first check if it is calculated and start
calculation if not found.

Am 18.04.2018 um 19:32 schrieb Eugene Zelenko via Phabricator:

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D45679#1071116, @JonasToth wrote:

You are doing a great job and i learn new stuff :)

  • What do you think about having these functions in a class? Now, we need to recalculate and reanalyze the scope for every variable, multiple times (reference tracking). It would be nice to do it as lazy as possible and memorize the results. Especially addressing the use-case for the const-check, storing that a reference is not modified will save a lot of work = performance

It may be reasonable to have variables/data members dependencies graph and mark them as constant/non constant.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

JonasToth added inline comments.Apr 18 2018, 11:40 AM
clang-tidy/utils/ASTUtils.h
31 ↗(On Diff #142882)

Maybe you could add an Unknown kind, e.g. if the expression is not found or similar.

JonasToth added a comment.EditedApr 18 2018, 12:59 PM

Another note: https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/utils/DeclRefExprUtils.cpp
There is a isOnlyUsedAsConst with a slick implementation, using a set difference.

Edit: I used it to check, it has its shortcommings, too. It definitly fails for lambdas.

JonasToth added inline comments.Apr 18 2018, 1:50 PM
clang-tidy/utils/ASTUtils.cpp
226 ↗(On Diff #142882)

What about transitive references and pointers?
It seems that only references are checked, but if a pointer is taken through that reference and vice versa, is that tracked correctly?

233 ↗(On Diff #142882)

If the Exp is an array, that will not match array subscript?

Are there other similar cases?

shuaiwang updated this revision to Diff 143197.Apr 19 2018, 5:03 PM
shuaiwang marked 13 inline comments as done.
  • Moved out of ASTUtils and becomes a separte class ExprMutationAnalyzer (bikeshedding on name a bit.)
  • Reverted back to return bool instead of tri-valued enum, the meanings of Escaped and Modified are not well defined.
  • Cosmetic changes like spliting large function and reducing duplicates.
  • Addressed comments.

Regarding full dependency analysis, I think it's out of the scope of this change at least for now. We're only trying to find one possible point where the given Expr is (assumed to be) mutated. This is known to be useful at this point for use cases like "check whether const can be added" or "check whether a by-value copy can be replace with a const-by-ref capture". The analysis is also designed to be performed within a limited scope because we're mostly targeting code-style issues (for example, even if we can do the analysis across multiple functions within the same TU, it might still not be a good idea because at that point it's much less obvious to human readers.) Figuring out the full dependency will be more costly than what we're doing right now and would be overkill for the applications we'd like to apply to.

For isOnlyUsedAsConst, I'm intending to replace that with this (ref one of my oldest comment.)

clang-tidy/utils/ASTUtils.cpp
125 ↗(On Diff #142882)

Added test cases for this.
cxxConstructExpr is the only one I can think of that happens to not be covered by callExpr.

149 ↗(On Diff #142882)

Could you elaborate what do you mean here?

222 ↗(On Diff #142882)

Removed.

In case you wonder, I was intended to put a bit more information into Chain and here we're basically trying each element of LoopVars and see whether it's modified or not, because each recursive call appends to Chain we need to push the LoopVarStmt first before doing the recursive call, but if the recursive call concluded that the var is not modified, we need to pop the LoopVarStmt back out.
Anyway this is not removed.

226 ↗(On Diff #142882)

Yes.

As soon as an address is taken we assume it's modified. Currently we don't follow pointers.
Examples:
int a = 10;
&a; <-- taking address, assumed modification point

int b = 10;
int& c = b; <-- follow the reference
&c; <-- taking address, assumed modification point, propagates back to "b"

233 ↗(On Diff #142882)

If Exp is an array, we should first follow the array subscript expression and do a recursive call.

i.e.:
int x[2];
int& y = x[0];
y = 10;
isModified(x) -> isModified(x[0]) -> isModified(y) { return true /* because y = 10 */ }

clang-tidy/utils/ASTUtils.h
31 ↗(On Diff #142882)

Reverted back to just bool.

I like your refactoring very much and i think we have a state that is close to commit.

My clang-tidy check is already based on top of your functionality, but i can not work on it this weekend and next week is already busy for me. I decided to analysis values and references only for now, and work on pointer semantics later, because of some uncertainty how to handle different things.

Right now, nothing comes to my mind that might be missing. Maybe you could add small helpers and utilities, that take a varDecl and run the analysis (implemented in my check). That would move the last piece into this section :)

clang-tidy/utils/ASTUtils.cpp
125 ↗(On Diff #142882)

Are move and copy constructor covered by this?

149 ↗(On Diff #142882)

I think i was mostly irritated and it is irrelevant with the new semantics (which i think are good for now!).

222 ↗(On Diff #142882)

Ok.

233 ↗(On Diff #142882)

I was wondering about the ternaryOperator and if some suprises might arise from it.

Thats something i will investigate next week within the clang-tidy check.

JonasToth added a comment.EditedApr 20 2018, 3:12 AM

Something came to my mind:

conversion operators. Do they behave as normal overloaded operators? The tests should reflect both a const conversion and a modifying one.

Edit: Already covered

JonasToth added a comment.EditedApr 28 2018, 10:09 AM

There are false positives.

void false_positive() {
// FIXME False positive
int np_local0 = 42;               
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local0' of type 'int' can be declared const
const int &r0_np_local0 = np_local0;
int &r1_np_local0 = np_local0;
r1_np_local0 = 43;
}
// FIXME false positive
int np_local0 = 42;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local0' of type 'int' can be declared const
const int *const p0_np_local0 = &np_local0;
int *const p1_np_local0 = &np_local0;
*p1_np_local0 = 43;
// FIXME False positive
ConstNonConstClass np_local0;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local0' of type 'ConstNonConstClass' can be declared const

np_local0.constMethod();
np_local0.nonConstMethod();
// FIXME array access was modifying
ConstNonConstClass np_local0[2];
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local0' of type 'ConstNonConstClass [2]' can be declared const
np_local0[0].constMethod();
np_local0[1].constMethod();
np_local0[1].nonConstMethod();
// FIXME false positive
int np_local5[3] = {1, 2, 3};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local5' of type 'int [3]' can be declared const [cppcoreguidelines-const]
int &np_local6 = np_local5[1] < np_local5[2] ? np_local5[0] : np_local5[2];
np_local6 = 42;
// FIXME false positive
int np_local7[3] = {1, 2, 3};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local7' of type 'int [3]' can be declared const [cppcoreguidelines-const]
int *np_local8 = np_local7[1] < np_local7[2] ? &np_local7[0] : &np_local7[2];
*np_local8 = 42;
shuaiwang marked 11 inline comments as done.

Added an isMutated overload for Decl.
A few more test cases.

I migrated to the new Decl interface and adjusted my tests, there are no false positives left and I am not aware of more possible code constellation that would require testing.
If you have time you could check my tests, maybe you find something I missed.

Otherwise this is LGTM from me, but the reviewers must of course accept it (@aaron.ballman, @alexfh)

aaron.ballman added inline comments.May 2 2018, 11:18 AM
clang-tidy/utils/ExprMutationAnalyzer.cpp
43

Can elide the braces.

72

Don't use auto here.

83–84

Why does this not return the result of findMutation() like the other call above?

89

Should this also consider a DeclRefExpr to a volatile-qualified variable as a direct mutation?

What about using Expr::HasSideEffect()?

shuaiwang updated this revision to Diff 145121.May 3 2018, 5:24 PM
shuaiwang marked 4 inline comments as done.

Addressed review comments, notably added check for DeclRefExpr to volatile variables.

clang-tidy/utils/ExprMutationAnalyzer.cpp
83–84

find*Mutation is intended to return where the given Expr is mutated.
To make it a bit more useful, for Decl's I'm returning the DeclRefExpr and caller can chose to follow it to find where the DeclRefExpr is mutated if needed.
As a concrete example:

struct A { int x; };
struct B { A a; };
struct C { B b; };
C c;
C& c2 = c;
c2.b.a.x = 10;

If we start with DeclRefExpr to c we'll find mutation at a DeclRefExpr to c2, following that we can find the mutation Stmt being c2.b.a.x = 10.
Returning E here makes it possible to reveal intermediate checkpoint instead of directly saying c is mutated at c2.b.a.x = 10 which might be confusing.

89

Good catch about DeclRefExpr to volatile.

HasSideEffects means something different. Here find*Mutation means find a Stmt in ancestors that mutates the given Expr. HasSideEffects IIUC means whether anything is mutated by the Expr but doesn't care about what exactly is mutated.

shuaiwang retitled this revision from [clang-tidy] Add a helper function isModified, that checks whether an expression is modified within a statement. to [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement..May 3 2018, 5:26 PM
shuaiwang edited the summary of this revision. (Show Details)
JonasToth added inline comments.May 4 2018, 1:23 AM
clang-tidy/utils/ExprMutationAnalyzer.cpp
89

May I ask the exact semantics for volatile? I would add the test to my check, too.

It is possible to write const volatile int m = 42;, but if i understand correctly m could still change by hardware, or would this be UB?
If the const is missing, one must always assume external modification, right?

aaron.ballman added inline comments.May 4 2018, 5:59 AM
clang-tidy/utils/ExprMutationAnalyzer.cpp
89

HasSideEffects means something different. Here find*Mutation means find a Stmt in ancestors that mutates the given Expr. HasSideEffects IIUC means whether anything is mutated by the Expr but doesn't care about what exactly is mutated.

What I am getting at is that the code in findDirectMutation() seems to go through a lot of effort looking for expressions that have side effects and I was wondering if we could leverage that call instead of duplicating the effort.

89

May I ask the exact semantics for volatile? I would add the test to my check, too.

Basically, volatile objects can change in ways unknown to the implementation, so a volatile read must always perform an actual read (because the object's value may have changed since the last read) and a volatile write must always be performed as well (because writing a value may have further side effects if the object is backed by some piece of interesting hardware).

It is possible to write const volatile int m = 42;, but if i understand correctly m could still change by hardware, or would this be UB?

That could still be changed by hardware, and it would not be UB. For instance, the variable might be referencing some hardware sensor and so you can only read the values in (hence it's const) but the values may be changed out from under you (hence, it's not constant).

If the const is missing, one must always assume external modification, right?

You have to assume external modification regardless.

shuaiwang updated this revision to Diff 145313.May 4 2018, 3:36 PM

Revert "DeclRefExpr to volatile handling" part of last update.

shuaiwang marked 5 inline comments as done.May 4 2018, 3:49 PM

I've reverted the handling of DeclRefExpr to volatile after rethinking about it.

The purpose of this analyzer is to find whether the given Expr is mutated within a scope, but doesn't really care about external changes. In other words we're trying to find mutations explicitly written within the specified scope.

clang-tidy/utils/ExprMutationAnalyzer.cpp
89

Unfortunately I don't think HasSideEffects help in here. We're finding one particular side effect that is mutating the specified Expr so we need to have explicitly equalsNode(Exp) for all the matches. HasSideEffects can simply claim f(a, b) or even g() has side effects (given void f(Foo&, const Bar&), void g()) while we need to require a link to the specific Expr we're analyzing (i.e. f(a, b) is mutating a but not b)

I've reverted the handling of DeclRefExpr to volatile after rethinking about it.

The purpose of this analyzer is to find whether the given Expr is mutated within a scope, but doesn't really care about external changes. In other words we're trying to find mutations explicitly written within the specified scope.

Okay, that seems like a good reason to ignore volatile qualifiers. Thank you for the explanation!

clang-tidy/utils/ExprMutationAnalyzer.cpp
89

Ah, okay, thank you for the explanation.

I think you may want to look at the implementation for HasSideEffect() to identify other cases you may want to support. For instance, adding tests for statement expressions, paren expressions, bizarre things like VLA mutations in otherwise unevaluated contexts, etc.

shuaiwang updated this revision to Diff 146531.May 13 2018, 3:08 PM
shuaiwang marked 3 inline comments as done.

Handle unevaluated expressions.

aaron.ballman added inline comments.May 18 2018, 5:41 AM
clang-tidy/utils/ExprMutationAnalyzer.cpp
22–26

This can be replaced with return llvm::find(Node.capture_inits(), E);

35–39

I think these should be exposed as public AST matchers, as they're both generally useful. It can be done in a follow-up patch, or done before landing this patch, either is fine by me.

68

What about other unevaluated expressions, like typeof, _Generic, etc? You should search for ExpressionEvaluationContext::Unevaluated in Clang to see where we set up an unevaluated expression evaluation context to find all of the situations.

73

What is this trying to match with the typeLoc() matcher?

75–76

I think these are only approximations to testing whether it's unevaluated. For instance, won't this match these expressions, despite them being evaluated?

// C++
#include <typeinfo>

struct B {
  virtual ~B() = default;
};

struct D : B {};

B& get() {
  static B *b = new D;
  return *b;
}

void f() {
  (void)typeid(get()); // Evaluated, calls get()
}

/* C99 and later */
#include <stddef.h>

void f(size_t n) {
  (void)sizeof(int[++n]); // Evaluated, increments n
}
george.karpenkov resigned from this revision.May 30 2018, 4:30 PM
shuaiwang updated this revision to Diff 149659.Jun 3 2018, 4:15 PM
shuaiwang marked 5 inline comments as done.

Handle sizeof on VLA.
Added test case for typeof()
Added TODO for handling typeid and generic selection.

clang-tidy/utils/ExprMutationAnalyzer.cpp
35–39

Will leave as follow up patch.

68

I checked around and I think these are all we care about:

  • decltype/typeof
  • sizeof/alignof
  • noexcept
  • _Generic
  • typeid

I've added TODO for _Generic and typeid for now because I think those need a bit more work to implement, while at the same time not supporting them for now only creates false positives from isMutated which is what we prefer over false negatives.

73

Added comment to explain.

75–76

Fixed handling of sizeof(VLA)
Added TODO for typeid

shuaiwang updated this revision to Diff 149661.Jun 3 2018, 5:05 PM
shuaiwang marked 4 inline comments as done.

Handle typeid and generic selection.

Overestimated the work of supporting typeid and _Generic, done now.

aaron.ballman accepted this revision.Jun 5 2018, 12:42 AM

Aside from a minor commenting nit, this LGTM.

clang-tidy/utils/ExprMutationAnalyzer.cpp
79–82

I think this comment is stale now.

This revision is now accepted and ready to land.Jun 5 2018, 12:42 AM
shuaiwang updated this revision to Diff 150026.Jun 5 2018, 12:24 PM
shuaiwang marked an inline comment as done.

Remove stale comment

Thanks a lot for the review!

Could you also help commit this diff as well? Thanks!

@shuaiwang I can commit it in 2 hours for you if you want, after my lecture.

Hmm. SVN does not want  me to commit anything :/

I will retry later today, but maybe i cant commit.

Best, Jonas

Am 06.06.2018 um 10:47 schrieb Aaron Ballman:

Thanks a lot for the review!

Could you also help commit this diff as well? Thanks!

There are at least two previous accepted&committed differentials (D17586, D45702),
i think you can ask for commit bit now.

aaron.ballman closed this revision.Jun 13 2018, 7:46 AM

I've commit in r334604 with one minor change -- I added an include for <cctype> to the unit test so that std::isspace() would compile properly on all platforms.

aaron.ballman reopened this revision.Jun 13 2018, 8:07 AM

I had to revert due to failing tests. The revert was done in r334606 and this is an example of a failing bot: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/31500

This revision is now accepted and ready to land.Jun 13 2018, 8:07 AM
shuaiwang updated this revision to Diff 151244.Jun 13 2018, 2:02 PM

Don't include <typeinfo> in unit tests, should fix the test failures.

shuaiwang updated this revision to Diff 151245.Jun 13 2018, 2:05 PM

Add include <cctype> for std::isspace() (thanks aaron.ballman!)

I had to revert due to failing tests. The revert was done in r334606 and this is an example of a failing bot: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/31500

Apparently I can't include <typeinfo> in unit test cases (somehow that works on my machine :p)
Changed to just forward declare std::type_info instead.

It might be the case, that the test is run with -no-stdinc (or similar),
so the standard library is not available.

Am 13.06.2018 um 23:08 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45679#1131115, @aaron.ballman wrote:

I had to revert due to failing tests. The revert was done in r334606 and this is an example of a failing bot: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/31500

Apparently I can't include <typeinfo> in unit test cases (somehow that works on my machine :p)
Changed to just forward declare std::type_info instead.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

It might be the case, that the test is run with -no-stdinc (or similar),
so the standard library is not available.

Tests should be self-contained and must not depend on any system headers. Standard library implementations may differ, which would make tests brittle. Instead, tests should mock implementations of the APIs they work with (if necessary, multiple times - once for every distinct standard library version the check supports).

Could someone help commit this now that the build issue with header include is fixed? Thanks a lot!
(meanwhile I'm requesting commit access)

It might be the case, that the test is run with -no-stdinc (or similar),
so the standard library is not available.

Tests should be self-contained and must not depend on any system headers. Standard library implementations may differ, which would make tests brittle. Instead, tests should mock implementations of the APIs they work with (if necessary, multiple times - once for every distinct standard library version the check supports).

Yep. It doesn't include any headers anymore :)

This revision was automatically updated to reflect the committed changes.
alexfh added inline comments.Jun 28 2018, 5:14 AM
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
582

FYI, this test had to be fixed for a ps4 buildbot. See r335799 for details.

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

I think it should in theory be possible, though I need some advice about where exactly is the best place in clang.

I think clang/lib/Analysis is a good place.

I think it should in theory be possible, though I need some advice about where exactly is the best place in clang.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

lebedev.ri added inline comments.Sep 10 2018, 5:13 AM
clang-tidy/utils/ExprMutationAnalyzer.h
38

@shuaiwang, @JonasToth hi.
Is it very intentional that this findDeclMutation() is private?

Is there some other way i should be doing if i have a statement S,
a declRefExpr D, and i want to find the statement Q, which mutates
the underlying variable to which the declRefExpr D refers?

lebedev.ri added inline comments.Sep 10 2018, 5:17 AM
clang-tidy/utils/ExprMutationAnalyzer.h
38

(the statement Q within the statement S, of course)

lebedev.ri added inline comments.Sep 10 2018, 7:14 AM
clang-tidy/utils/ExprMutationAnalyzer.h
38

@shuaiwang after a disscussion about this in IRC with @JonasToth, i have filed https://bugs.llvm.org/show_bug.cgi?id=38888
But i'm failing to CC you there. Are you not registered in the bugzilla?

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

FYI I haven't forgot about this, I'll look into doing it after a few pending changes are in.

clang-tidy/utils/ExprMutationAnalyzer.h
38

There's no real reason findDeclMutation is private other than that there wasn't a use case :)
Feel free to make it public if you find it useful that way.

I'll take a look at the bug, thanks for reporting it!
(I don't have an account there yet, I'm requesting one right now, will follow up in the bug)

@shuaiwang What are you working on next? Do you have an idea on how we
will handle the pointee of a pointer? That would be very interesting for
my const-correctness check. if you need help with anything, just say so.
I would like to support

Am 10.09.2018 um 18:49 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45679#1183116, @george.karpenkov wrote:

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

FYI I haven't forgot about this, I'll look into doing it after a few pending changes are in.

Comment at: clang-tidy/utils/ExprMutationAnalyzer.h:38
+ const Stmt *findDeclMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
+ const Stmt *findDeclMutation(const Decl *Dec);

+

lebedev.ri wrote:

lebedev.ri wrote:

lebedev.ri wrote:

@shuaiwang, @JonasToth hi.
Is it very intentional that this findDeclMutation() is private?

Is there some other way i should be doing if i have a statement S,
a declRefExpr D, and i want to find the statement Q, which mutates
the underlying variable to which the declRefExpr D refers?

(the statement Q within the statement S, of course)

@shuaiwang after a disscussion about this in IRC with @JonasToth, i have filed https://bugs.llvm.org/show_bug.cgi?id=38888
But i'm failing to CC you there. Are you not registered in the bugzilla?

There's no real reason findDeclMutation is private other than that there wasn't a use case :)
Feel free to make it public if you find it useful that way.

I'll take a look at the bug, thanks for reporting it!
(I don't have an account there yet, I'm requesting one right now, will follow up in the bug)

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

I have a rough idea about how findPointeeMutation would look like, I'm pretty sure I can use a lot of your help :)
My current plan:

  • Finish the few existing pending changes
  • Move the analyzer to clang/lib/Analysis (likely remove PseudoConstantAnalysis there as well since it appears to be dead code for years)
  • Implement findPointeeMutation

@shuaiwang What are you working on next? Do you have an idea on how we
will handle the pointee of a pointer? That would be very interesting for
my const-correctness check. if you need help with anything, just say so.
I would like to support

Am 10.09.2018 um 18:49 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45679#1183116, @george.karpenkov wrote:

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

FYI I haven't forgot about this, I'll look into doing it after a few pending changes are in.

Comment at: clang-tidy/utils/ExprMutationAnalyzer.h:38
+ const Stmt *findDeclMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
+ const Stmt *findDeclMutation(const Decl *Dec);

+

lebedev.ri wrote:

lebedev.ri wrote:

lebedev.ri wrote:

@shuaiwang, @JonasToth hi.
Is it very intentional that this findDeclMutation() is private?

Is there some other way i should be doing if i have a statement S,
a declRefExpr D, and i want to find the statement Q, which mutates
the underlying variable to which the declRefExpr D refers?

(the statement Q within the statement S, of course)

@shuaiwang after a disscussion about this in IRC with @JonasToth, i have filed https://bugs.llvm.org/show_bug.cgi?id=38888
But i'm failing to CC you there. Are you not registered in the bugzilla?

There's no real reason findDeclMutation is private other than that there wasn't a use case :)
Feel free to make it public if you find it useful that way.

I'll take a look at the bug, thanks for reporting it!
(I don't have an account there yet, I'm requesting one right now, will follow up in the bug)

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

I have a rough idea about how findPointeeMutation would look like, I'm pretty sure I can use a lot of your help :)
My current plan:

  • Finish the few existing pending changes
  • Move the analyzer to clang/lib/Analysis (likely remove PseudoConstantAnalysis there as well since it appears to be dead code for years)
  • Implement findPointeeMutation

Sounds like a solid plan. I think the clang-tidy/utils/DeclRefExprUtils.{h,cpp} are not necessary anymore, too. They are used in 2 clang-tidy checks. Maybe i can remove them as well :)