[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
JonasToth added a comment.
In https://reviews.llvm.org/D45679#1229176, @shuaiwang wrote:
> 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 :)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang added a comment. 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` In https://reviews.llvm.org/D45679#1229071, @JonasToth wrote: > @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 > > 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=3 > >> 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 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
JonasToth added a comment. @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 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=3 >> 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 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
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 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=3 > 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
lebedev.ri added inline comments. Comment at: clang-tidy/utils/ExprMutationAnalyzer.h:38 + const Stmt *findDeclMutation(ArrayRef Matches); + const Stmt *findDeclMutation(const Decl *Dec); + 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=3 But i'm failing to CC you there. Are you not registered in the bugzilla? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
lebedev.ri added inline comments. Comment at: clang-tidy/utils/ExprMutationAnalyzer.h:38 + const Stmt *findDeclMutation(ArrayRef Matches); + const Stmt *findDeclMutation(const Decl *Dec); + 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) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
lebedev.ri added inline comments. Herald added a subscriber: Szelethus. Comment at: clang-tidy/utils/ExprMutationAnalyzer.h:38 + const Stmt *findDeclMutation(ArrayRef Matches); + const Stmt *findDeclMutation(const Decl *Dec); + @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? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
JonasToth added a comment. 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 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
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. 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
george.karpenkov added a comment. @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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
alexfh added inline comments.
Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:582
+
+ AST = tooling::buildASTFromCode("namespace std { class type_info; }"
+ "void f() { int x; typeid(x = 10); }");
FYI, this test had to be fixed for a ps4 buildbot. See r335799 for details.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE335736: [clang-tidy] Add ExprMutationAnalyzer, that
analyzes whether an expression is… (authored by alexfh, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D45679?vs=151245&id=153087#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/ExprMutationAnalyzer.cpp
clang-tidy/utils/ExprMutationAnalyzer.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
Index: clang-tidy/utils/ExprMutationAnalyzer.cpp
===
--- clang-tidy/utils/ExprMutationAnalyzer.cpp
+++ clang-tidy/utils/ExprMutationAnalyzer.cpp
@@ -0,0 +1,260 @@
+//===-- ExprMutationAnalyzer.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 "ExprMutationAnalyzer.h"
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/STLExtras.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+using namespace ast_matchers;
+
+namespace {
+
+AST_MATCHER_P(LambdaExpr, hasCaptureInit, const Expr *, E) {
+ return llvm::is_contained(Node.capture_inits(), E);
+}
+
+AST_MATCHER_P(CXXForRangeStmt, hasRangeStmt,
+ ast_matchers::internal::Matcher, InnerMatcher) {
+ const DeclStmt *const Range = Node.getRangeStmt();
+ return InnerMatcher.matches(*Range, Finder, Builder);
+}
+
+const ast_matchers::internal::VariadicDynCastAllOfMatcher
+cxxTypeidExpr;
+
+AST_MATCHER(CXXTypeidExpr, isPotentiallyEvaluated) {
+ return Node.isPotentiallyEvaluated();
+}
+
+const ast_matchers::internal::VariadicDynCastAllOfMatcher
+cxxNoexceptExpr;
+
+const ast_matchers::internal::VariadicDynCastAllOfMatcher
+genericSelectionExpr;
+
+AST_MATCHER_P(GenericSelectionExpr, hasControllingExpr,
+ ast_matchers::internal::Matcher, InnerMatcher) {
+ return InnerMatcher.matches(*Node.getControllingExpr(), Finder, Builder);
+}
+
+const auto nonConstReferenceType = [] {
+ return referenceType(pointee(unless(isConstQualified(;
+};
+
+} // namespace
+
+const Stmt *ExprMutationAnalyzer::findMutation(const Expr *Exp) {
+ const auto Memoized = Results.find(Exp);
+ if (Memoized != Results.end())
+return Memoized->second;
+
+ if (isUnevaluated(Exp))
+return Results[Exp] = nullptr;
+
+ for (const auto &Finder : {&ExprMutationAnalyzer::findDirectMutation,
+ &ExprMutationAnalyzer::findMemberMutation,
+ &ExprMutationAnalyzer::findArrayElementMutation,
+ &ExprMutationAnalyzer::findCastMutation,
+ &ExprMutationAnalyzer::findRangeLoopMutation,
+ &ExprMutationAnalyzer::findReferenceMutation}) {
+if (const Stmt *S = (this->*Finder)(Exp))
+ return Results[Exp] = S;
+ }
+
+ return Results[Exp] = nullptr;
+}
+
+bool ExprMutationAnalyzer::isUnevaluated(const Expr *Exp) {
+ return selectFirst(
+ "expr",
+ match(
+ findAll(
+ expr(equalsNode(Exp),
+ anyOf(
+ // `Exp` is part of the underlying expression of
+ // decltype/typeof if it has an ancestor of
+ // typeLoc.
+ hasAncestor(typeLoc(unless(
+ hasAncestor(unaryExprOrTypeTraitExpr(),
+ hasAncestor(expr(anyOf(
+ // `UnaryExprOrTypeTraitExpr` is unevaluated
+ // unless it's sizeof on VLA.
+ unaryExprOrTypeTraitExpr(unless(sizeOfExpr(
+ hasArgumentOfType(variableArrayType(),
+ // `CXXTypeidExpr` is unevaluated unless it's
+ // applied to an expression of glvalue of
+ // polymorphic class type.
+ cxxTypeidExpr(
+ unless(isPotentiallyEvaluated())),
+ // The controlling expression of
+ // `GenericSelectionExpr` is unevaluated.
+ genericSelectionExpr(hasControllingExpr(
+ hasDescendant(equalsNode(Exp,
+ cxxNoexceptExpr())
+ .bind("expr")),
+ *Stm, *Context)) != nullptr;
+}
+
+const Stmt *
+ExprMutationAnalyzer::fi
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang added a comment. Could someone help commit this now that the build issue with header include is fixed? Thanks a lot! (meanwhile I'm requesting commit access) In https://reviews.llvm.org/D45679#1132327, @alexfh wrote: > In https://reviews.llvm.org/D45679#1132086, @JonasToth wrote: > > > 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 :) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
alexfh added a comment. In https://reviews.llvm.org/D45679#1132086, @JonasToth wrote: > 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). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
JonasToth added a comment. 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 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 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
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 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang updated this revision to Diff 151245.
shuaiwang added a comment.
Add include for std::isspace() (thanks aaron.ballman!)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/ExprMutationAnalyzer.cpp
clang-tidy/utils/ExprMutationAnalyzer.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,609 @@
+//===-- ExprMutationAnalyzerTest.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 "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+ return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+ return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ return utils::ExprMutationAnalyzer(S, &AST->getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ SmallVector Chain;
+ utils::ExprMutationAnalyzer Analyzer(S, &AST->getASTContext());
+ for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
+const Stmt *By = Analyzer.findMutation(E);
+std::string buffer;
+llvm::raw_string_ostream stream(buffer);
+By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+Chain.push_back(StringRef(stream.str()).trim().str());
+E = dyn_cast(By);
+ }
+ return Chain;
+}
+
+std::string removeSpace(std::string s) {
+ s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+ s.end());
+ return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+ const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+ const std::string ModExpr = "x " + GetParam() + " 10";
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+ "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+ const std::string ModExpr = GetParam();
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+}
+
+TEST(ExprMutationAnalyzerTest, ConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTe
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang updated this revision to Diff 151244.
shuaiwang added a comment.
Don't include in unit tests, should fix the test failures.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/ExprMutationAnalyzer.cpp
clang-tidy/utils/ExprMutationAnalyzer.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,608 @@
+//===-- ExprMutationAnalyzerTest.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 "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+ return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+ return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ return utils::ExprMutationAnalyzer(S, &AST->getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ SmallVector Chain;
+ utils::ExprMutationAnalyzer Analyzer(S, &AST->getASTContext());
+ for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
+const Stmt *By = Analyzer.findMutation(E);
+std::string buffer;
+llvm::raw_string_ostream stream(buffer);
+By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+Chain.push_back(StringRef(stream.str()).trim().str());
+E = dyn_cast(By);
+ }
+ return Chain;
+}
+
+std::string removeSpace(std::string s) {
+ s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+ s.end());
+ return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+ const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+ const std::string ModExpr = "x " + GetParam() + " 10";
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+ "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+ const std::string ModExpr = GetParam();
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+}
+
+TEST(ExprMutationAnalyzerTest, ConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, Non
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
aaron.ballman reopened this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. 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 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
aaron.ballman closed this revision. aaron.ballman added a comment. I've commit in r334604 with one minor change -- I added an include for to the unit test so that `std::isspace()` would compile properly on all platforms. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
lebedev.ri added a comment. In https://reviews.llvm.org/D45679#1122826, @shuaiwang wrote: > 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 (https://reviews.llvm.org/D17586, https://reviews.llvm.org/D45702), i think you can ask for commit bit now. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
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: > On Wed, Jun 6, 2018 at 4:40 AM Jonas Toth via Phabricator > wrote: >> JonasToth added a comment. >> >> @shuaiwang I can commit it in 2 hours for you if you want, after my lecture. > That's sooner than my "I can commit next week." :-D > > ~Aaron > >> >> Repository: >> rCTE Clang Tools Extra >> >> https://reviews.llvm.org/D45679 >> >> >> ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
On Wed, Jun 6, 2018 at 4:40 AM Jonas Toth via Phabricator wrote: > > JonasToth added a comment. > > @shuaiwang I can commit it in 2 hours for you if you want, after my lecture. That's sooner than my "I can commit next week." :-D ~Aaron > > > Repository: > rCTE Clang Tools Extra > > https://reviews.llvm.org/D45679 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
JonasToth added a comment. @shuaiwang I can commit it in 2 hours for you if you want, after my lecture. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang added a comment. Thanks a lot for the review! Could you also help commit this diff as well? Thanks! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang updated this revision to Diff 150026.
shuaiwang marked an inline comment as done.
shuaiwang added a comment.
Remove stale comment
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/ExprMutationAnalyzer.cpp
clang-tidy/utils/ExprMutationAnalyzer.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,608 @@
+//===-- ExprMutationAnalyzerTest.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 "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+ return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+ return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ return utils::ExprMutationAnalyzer(S, &AST->getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ SmallVector Chain;
+ utils::ExprMutationAnalyzer Analyzer(S, &AST->getASTContext());
+ for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
+const Stmt *By = Analyzer.findMutation(E);
+std::string buffer;
+llvm::raw_string_ostream stream(buffer);
+By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+Chain.push_back(StringRef(stream.str()).trim().str());
+E = dyn_cast(By);
+ }
+ return Chain;
+}
+
+std::string removeSpace(std::string s) {
+ s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+ s.end());
+ return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+ const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+ const std::string ModExpr = "x " + GetParam() + " 10";
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+ "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+ const std::string ModExpr = GetParam();
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+}
+
+TEST(ExprMutationAnalyzerTest, ConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from a minor commenting nit, this LGTM. Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:78-81 + // TODO: also handle these cases: + // - `Exp` is the ControllingExpr of a GenericSelectionExpr + // - `Exp` is within an expression (other than glvalue of a polymorphic class + //type) operand of typeid. I think this comment is stale now. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang added a comment. Overestimated the work of supporting typeid and _Generic, done now. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45679 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang updated this revision to Diff 149661.
shuaiwang marked 4 inline comments as done.
shuaiwang added a comment.
Handle typeid and generic selection.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/ExprMutationAnalyzer.cpp
clang-tidy/utils/ExprMutationAnalyzer.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,608 @@
+//===-- ExprMutationAnalyzerTest.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 "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+ return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+ return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ return utils::ExprMutationAnalyzer(S, &AST->getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ SmallVector Chain;
+ utils::ExprMutationAnalyzer Analyzer(S, &AST->getASTContext());
+ for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
+const Stmt *By = Analyzer.findMutation(E);
+std::string buffer;
+llvm::raw_string_ostream stream(buffer);
+By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+Chain.push_back(StringRef(stream.str()).trim().str());
+E = dyn_cast(By);
+ }
+ return Chain;
+}
+
+std::string removeSpace(std::string s) {
+ s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+ s.end());
+ return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+ const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+ const std::string ModExpr = "x " + GetParam() + " 10";
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+ "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+ const std::string ModExpr = GetParam();
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+}
+
+TEST(ExprMutationAnalyzerTest, ConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMuta
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang added inline comments.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:34-38
+const ast_matchers::internal::VariadicDynCastAllOfMatcher
+cxxTypeidExpr;
+
+const ast_matchers::internal::VariadicDynCastAllOfMatcher
+cxxNoexceptExpr;
aaron.ballman wrote:
> 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.
Will leave as follow up patch.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:67
+
+bool ExprMutationAnalyzer::isUnevaluated(const Expr *Exp) {
+ return selectFirst(
aaron.ballman wrote:
> 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.
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.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:72
+ findAll(expr(equalsNode(Exp),
+ anyOf(hasAncestor(typeLoc()),
+hasAncestor(expr(anyOf(
aaron.ballman wrote:
> What is this trying to match with the `typeLoc()` matcher?
Added comment to explain.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:74-75
+hasAncestor(expr(anyOf(
+unaryExprOrTypeTraitExpr(),
+cxxTypeidExpr(), cxxNoexceptExpr())
+ .bind("expr")),
aaron.ballman wrote:
> 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
>
> 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
>
> void f(size_t n) {
> (void)sizeof(int[++n]); // Evaluated, increments n
> }
> ```
Fixed handling of sizeof(VLA)
Added TODO for typeid
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang updated this revision to Diff 149659.
shuaiwang marked 5 inline comments as done.
shuaiwang added a comment.
Handle sizeof on VLA.
Added test case for typeof()
Added TODO for handling typeid and generic selection.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/ExprMutationAnalyzer.cpp
clang-tidy/utils/ExprMutationAnalyzer.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,591 @@
+//===-- ExprMutationAnalyzerTest.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 "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+ return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+ return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ return utils::ExprMutationAnalyzer(S, &AST->getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ SmallVector Chain;
+ utils::ExprMutationAnalyzer Analyzer(S, &AST->getASTContext());
+ for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
+const Stmt *By = Analyzer.findMutation(E);
+std::string buffer;
+llvm::raw_string_ostream stream(buffer);
+By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+Chain.push_back(StringRef(stream.str()).trim().str());
+E = dyn_cast(By);
+ }
+ return Chain;
+}
+
+std::string removeSpace(std::string s) {
+ s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+ s.end());
+ return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+ const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+ const std::string ModExpr = "x " + GetParam() + " 10";
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+ "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+ const std::string ModExpr = GetParam();
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+}
+
+TEST(ExprMutationAnalyzerTest, ConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
aaron.ballman added inline comments.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:21-25
+ for (const auto *Init : Node.capture_inits()) {
+if (Init == E)
+ return true;
+ }
+ return false;
This can be replaced with `return llvm::find(Node.capture_inits(), E);`
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:34-38
+const ast_matchers::internal::VariadicDynCastAllOfMatcher
+cxxTypeidExpr;
+
+const ast_matchers::internal::VariadicDynCastAllOfMatcher
+cxxNoexceptExpr;
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.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:67
+
+bool ExprMutationAnalyzer::isUnevaluated(const Expr *Exp) {
+ return selectFirst(
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.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:72
+ findAll(expr(equalsNode(Exp),
+ anyOf(hasAncestor(typeLoc()),
+hasAncestor(expr(anyOf(
What is this trying to match with the `typeLoc()` matcher?
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:74-75
+hasAncestor(expr(anyOf(
+unaryExprOrTypeTraitExpr(),
+cxxTypeidExpr(), cxxNoexceptExpr())
+ .bind("expr")),
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
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
void f(size_t n) {
(void)sizeof(int[++n]); // Evaluated, increments n
}
```
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang updated this revision to Diff 146531.
shuaiwang marked 3 inline comments as done.
shuaiwang added a comment.
Handle unevaluated expressions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/ExprMutationAnalyzer.cpp
clang-tidy/utils/ExprMutationAnalyzer.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,574 @@
+//===-- ExprMutationAnalyzerTest.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 "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+ return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+ return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ return utils::ExprMutationAnalyzer(S, &AST->getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ SmallVector Chain;
+ utils::ExprMutationAnalyzer Analyzer(S, &AST->getASTContext());
+ for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
+const Stmt *By = Analyzer.findMutation(E);
+std::string buffer;
+llvm::raw_string_ostream stream(buffer);
+By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+Chain.push_back(StringRef(stream.str()).trim().str());
+E = dyn_cast(By);
+ }
+ return Chain;
+}
+
+std::string removeSpace(std::string s) {
+ s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+ s.end());
+ return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+ const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+ const std::string ModExpr = "x " + GetParam() + " 10";
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+ "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+ const std::string ModExpr = GetParam();
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+}
+
+TEST(ExprMutationAnalyzerTest, ConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationA
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
aaron.ballman added a comment.
In https://reviews.llvm.org/D45679#1088696, @shuaiwang wrote:
> 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!
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+ // LHS of any assignment operators.
shuaiwang wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > JonasToth wrote:
> > > > shuaiwang wrote:
> > > > > aaron.ballman wrote:
> > > > > > Should this also consider a DeclRefExpr to a volatile-qualified
> > > > > > variable as a direct mutation?
> > > > > >
> > > > > > What about using `Expr::HasSideEffect()`?
> > > > > 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.
> > > > 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?
> > > > 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.
> > > 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.
> 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`)
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang marked 5 inline comments as done.
shuaiwang added a comment.
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.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+ // LHS of any assignment operators.
aaron.ballman wrote:
> aaron.ballman wrote:
> > JonasToth wrote:
> > > shuaiwang wrote:
> > > > aaron.ballman wrote:
> > > > > Should this also consider a DeclRefExpr to a volatile-qualified
> > > > > variable as a direct mutation?
> > > > >
> > > > > What about using `Expr::HasSideEffect()`?
> > > > 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.
> > > 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?
> > > 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.
> > 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.
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`)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
shuaiwang updated this revision to Diff 145313.
shuaiwang added a comment.
Revert "DeclRefExpr to volatile handling" part of last update.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
Files:
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/ExprMutationAnalyzer.cpp
clang-tidy/utils/ExprMutationAnalyzer.h
unittests/clang-tidy/CMakeLists.txt
unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,554 @@
+//===-- ExprMutationAnalyzerTest.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 "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+ return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+ return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ return utils::ExprMutationAnalyzer(S, &AST->getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl &Results, ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ SmallVector Chain;
+ utils::ExprMutationAnalyzer Analyzer(S, &AST->getASTContext());
+ for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
+const Stmt *By = Analyzer.findMutation(E);
+std::string buffer;
+llvm::raw_string_ostream stream(buffer);
+By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+Chain.push_back(StringRef(stream.str()).trim().str());
+E = dyn_cast(By);
+ }
+ return Chain;
+}
+
+std::string removeSpace(std::string s) {
+ s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+ s.end());
+ return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+ const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+ const std::string ModExpr = "x " + GetParam() + " 10";
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+ "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+ const std::string ModExpr = GetParam();
+ const auto AST =
+ tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+}
+
+TEST(ExprMutationAnalyzerTest, ConstMemberFunc) {
+ const auto AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest,
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
aaron.ballman added inline comments.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+ // LHS of any assignment operators.
JonasToth wrote:
> shuaiwang wrote:
> > aaron.ballman wrote:
> > > Should this also consider a DeclRefExpr to a volatile-qualified variable
> > > as a direct mutation?
> > >
> > > What about using `Expr::HasSideEffect()`?
> > 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.
> 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?
> 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.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+ // LHS of any assignment operators.
aaron.ballman wrote:
> JonasToth wrote:
> > shuaiwang wrote:
> > > aaron.ballman wrote:
> > > > Should this also consider a DeclRefExpr to a volatile-qualified
> > > > variable as a direct mutation?
> > > >
> > > > What about using `Expr::HasSideEffect()`?
> > > 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.
> > 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?
> > 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.
> 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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.
JonasToth added inline comments.
Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:88
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+ // LHS of any assignment operators.
shuaiwang wrote:
> aaron.ballman wrote:
> > Should this also consider a DeclRefExpr to a volatile-qualified variable as
> > a direct mutation?
> >
> > What about using `Expr::HasSideEffect()`?
> 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.
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?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45679
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
