shuaiwang updated this revision to Diff 145121.
shuaiwang marked 4 inline comments as done.
shuaiwang added a comment.
Addressed review comments, notably added check for DeclRefExpr to volatile
variables.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
shuaiwang added inline comments.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:82-83
+const auto *E = RefNodes.getNodeAs("expr");
+if (findMutation(E))
+ return E;
+ }
aaron.ballman wrote:
> Why does this not return the result of
aaron.ballman added inline comments.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:42
+ const auto Memoized = Results.find(Exp);
+ if (Memoized != Results.end()) {
+return Memoized->second;
Can elide the braces.
Comment at:
JonasToth added a comment.
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
shuaiwang updated this revision to Diff 144490.
shuaiwang marked 11 inline comments as done.
shuaiwang added a comment.
Added an isMutated overload for Decl.
A few more test cases.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/CMakeLists.txt
JonasToth added a comment.
There is a false positive with the reference checking. Could you please take a
look at it? I could not find the reason from inspecting your code.
void false_positive() {
// FIXME False positive
int np_local0 = 42;
// CHECK-MESSAGES:
JonasToth added a comment.
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
JonasToth added a comment.
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
shuaiwang added a comment.
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
shuaiwang updated this revision to Diff 143197.
shuaiwang marked 13 inline comments as done.
shuaiwang added a comment.
- 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
JonasToth added inline comments.
Comment at: clang-tidy/utils/ASTUtils.cpp:226
+
+ // If 'Exp' is bound to a non-const reference, check all declRefExpr to that.
+ const auto Refs = match(
What about transitive references and pointers?
It seems that only
JonasToth added a comment.
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.
Repository:
rCTE Clang Tools Extra
JonasToth added inline comments.
Comment at: clang-tidy/utils/ASTUtils.h:31
+
+enum ExprModificationKind {
+ EMK_NotModified, /// The Expr is neither modified nor escaped.
Maybe you could add an `Unknown` kind, e.g. if the expression is not found or
similar.
JonasToth added a comment.
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
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
JonasToth added a comment.
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).
shuaiwang updated this revision to Diff 142882.
shuaiwang added a comment.
Better range for loop handling.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/ASTUtils.cpp
clang-tidy/utils/ASTUtils.h
unittests/clang-tidy/CMakeLists.txt
shuaiwang updated this revision to Diff 142878.
shuaiwang added a comment.
Handle range-based for loop
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/ASTUtils.cpp
clang-tidy/utils/ASTUtils.h
unittests/clang-tidy/CMakeLists.txt
shuaiwang updated this revision to Diff 142870.
shuaiwang edited the summary of this revision.
shuaiwang added a comment.
Handle casts, ternary operator and lambdas.
Also optionally returns a chain of Stmt giving more details about how the
conclusion is reached.
Repository:
rCTE Clang Tools
shuaiwang added a comment.
In https://reviews.llvm.org/D45679#1069754, @Eugene.Zelenko wrote:
> In https://reviews.llvm.org/D45679#1069638, @JonasToth wrote:
>
> > *hust* /llvm/tools/clang/lib/Analysis/PseudoConstantAnalysis.cpp
> >
> > I will check this one first, before we get crazy and
Eugene.Zelenko added a comment.
In https://reviews.llvm.org/D45679#1069638, @JonasToth wrote:
> *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
JonasToth added a comment.
*hust* /llvm/tools/clang/lib/Analysis/PseudoConstantAnalysis.cpp
I will check this one first, before we get crazy and implemented something
twice.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
JonasToth added a comment.
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`
shuaiwang marked an inline comment as done.
shuaiwang added a comment.
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
24 matches
Mail list logo
