[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:32
                                                      check::PostCall> {
-  CallDescription CStrFn;
+  const llvm::SmallVector<CallDescription, 4> CStrFnFamily = {
+    {"std::basic_string<char>::c_str"}, {"std::basic_string<char32_t>::c_str"},
----------------
NoQ wrote:
> xazax.hun wrote:
> > I am not sure if this is the right solution in case of this check. We should track `c_str` calls regardless of what the template parameter is, so supporting any instantiation of `basic_string` is desired. This might not be the case, however, for other checks. 
> > 
> > If we think it is more likely we do not care about the template arguments, maybe a separate API could be used, where we pass the qualified name of the class separately without the template arguments.
> > Alternatively, we could use matches name so the users could use regexps.
> > 
> > At this point I also wonder what isCalled API gives us compared to matchers? Maybe it is more convenient to use than calling a `match`. Also, isCalled API has an IdentifierInfo cached which could be used for relatively efficient checks.
> > 
> > @NoQ what do you think?
> > 
> > 
> I agree that it's better to match the chain of classes and namespaces (in as much detail as the user cares to provide) and discard template parameters.
> 
> For example, i wish that a `CallDescription` that's defined as `{"std", "basic_string", "c_str"}` would be able to match both `std::string::c_str()` and `std::__1::string::c_str()`.
Yea, checker writers may can't provide all the template arguments, and it's not convenient to use!

I will try to give a better solution!


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:65
   auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
   if (TypeDecl->getName() != "basic_string")
     return;
----------------
xazax.hun wrote:
> If we check for fully qualified names, this check might be redundant.
Right, thanks!


================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:273
+  auto Matches =
+      match(namedDecl(hasName(CD.FuncName)).bind("match_qualified_name"), *ND,
+            LCtx->getAnalysisDeclContext()->getASTContext());
----------------
xazax.hun wrote:
> Doesn't match also traverse the subtree of the AST? We only need to check if that particular node matches the qualified name. I wonder if we could, during the traversal, find another node that is a match unintentionally.
Yea, this maybe a problem, I will test whether this is going to happen and give a fine-grained match. Thanks!



Repository:
  rC Clang

https://reviews.llvm.org/D48027