JonasToth added inline comments.
================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = tooling::buildASTFromCode(
----------------
shuaiwang wrote:
> JonasToth wrote:
> > shuaiwang wrote:
> > > JonasToth wrote:
> > > > JonasToth wrote:
> > > > > I feel that there a multiple tests missing:
> > > > >
> > > > > - multiple levels of pointers `int ***`, `int * const *`
> > > > > - pointers to references `int &*`
> > > > > - references to pointers `int *&`
> > > > > - ensure that having a const pointer does no influence the pointee
> > > > > analysis `int * const p = &i; *p = 42;`
> > > > > - a class with `operator*` + `operator->` const/non-const and the
> > > > > analysis for pointers to that class
> > > > > - pointer returned from a function
> > > > > - non-const reference returned
> > > > > ```
> > > > > int& foo(int *p) {
> > > > > return *p;
> > > > > }
> > > > > ```
> > > > >
> > > > for the multi-level pointer mutation: it would be enough to test, that
> > > > the second layer is analyzed properly, and that the `int * >const< *`
> > > > would be detected.
> > > Added except for:
> > > - Anything that requires following a dereference, we need
> > > `findPointeeDerefMutation` for that.
> > > - Pointer to a class with `operator*` + `operator->`, I think those two
> > > operators doesn't matter, there's no way to accidentally invoke them from
> > > a pointer.
> > >
> > But we want to analyze smart pointers in the future as well, not? It would
> > be good to already prepare that in the testing department.
> > Or would the nature of `operator*` already make that happen magically?
> Yes we'll handle smart pointers, and we'll handle that in
> `findPointeeDerefMutation`, basically it'll look like:
> ```
> if (native pointer && derefed with *) findMutation(deref expr)
> if (smart pointer && called operator*) findMutation(operator call expr)
> if (smart pointer && called operator->) findPointeeMutation(operator call
> expr)
> ```
> I think it would be more clear if we can match the implementation progress
> with unit test cases as that shows what kind of cases starts to be supported
> by the change.
Ok.
================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:61
+ ASTUnit *AST) {
+ const auto *const S = selectFirst<Stmt>("stmt", Results);
+ const auto *const E = selectFirst<Expr>("expr", Results);
----------------
Is there a reason why the pointer is marked `const`? Usually this is not done
in the codebase and I feel consistency is key (similar to `const` for values).
================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:90
+ ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+ const Stmt *By = Analyzer.findPointeeMutation(E);
+ std::string buffer;
----------------
You can put this line 2 lines below, 'initialize late'
================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:639
EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "return x;");
----------------
a nice test to add would be `const int* f() { int* x; return x; }.
Repository:
rC Clang
https://reviews.llvm.org/D52219
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
