Differential D48027
[analyzer] Improve `CallDescription` to handle c++ method. Authored by MTC on Jun 11 2018, 7:39 AM.
Details CallDecription can only handle function for the time being. If we want to match c++ method, we can only use method name to match and can't improve the matching accuracy through the qualifiers. This patch add the support for QualifiedName matching to improve the matching accuracy.
Diff Detail
Event TimelineComment Actions The implementation is not complicated, the difficulty is that there is no good way to get the qualified name without template arguments. For std::basic_string::c_str(), its qualified name may be std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::c_str, it is almost impossible for users to provide such a name. So one possible implementation is to use std, basic_string and c_str to match in the std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::c_str sequentially. Comment Actions Having C++ support would be awesome! While I do agree matching is not trivial with qualified names, this problem is already solved with AST matchers. I wonder if using matchers or reusing the HasNameMatcher class would make this easier. That code path is already well tested and optimized. Comment Actions Thank you for pointing out the direction, xazax.hun! I will looking into the matchers and try to give a better solution. Comment Actions
Comment Actions Sorry for the long long delay, I was on the Dragon Boat Festival a few days ago. This update has two parts:
There are two remain issues:
Comment Actions Thanks for your review, NoQ!
Comment Actions Hmm, instead of using the match function which will traverse the ast, we could use the HasNameMatcher class which can be invoked on only one node. That class is in an internal namespace though, so I think the best would be to ask the maintainers whether it is ok to use that class externally, or were whe should put the functionality we need. Comment Actions @xazax.hun Thanks for your tips! After some investigation, MatchFinder::match just traverse one ASTNode, that means match(namedDecl(HasNameMatcher())) and match(namedDecl(matchesName())) both not traverse children. So there are three ways to match the specified AST node.
In this update, I use the third way to match the specified AST node. This is simpler than I thought. In addition, add a redundant constructor that is only used to construct a CallDescription with one name, this avoids the modification of the existing checker, like BlockInCriticalSectionChecker.cpp. Any suggestions? Comment Actions Generally looks good, I only wonder if this works well with inline namespaces. Could you test? Probably it will. Comment Actions
Comment Actions Thank you for reminding me! Yea, this can handle inline namespaces. However this approach has limit. Given the code below, we cannot distinguish whether the basic_string is user-defined struct or namespace. That's means when the user provide {"std", "basic_string", "append"}, we can only know the qualified name of the call sequentially contains std, basic_string, append. We don't know if these names come from RecordDecl or NamespaceDecl. namespace std {
namespace basic_string {
struct A {
void append() {}
};
}
}
void foo() {
std::basic_string::A a;
a.append(); // Match
}@rnkovacs What do you think? Can this approach meet InnerPointerChecker's needs? Comment Actions Sorry for the delay, I think this is OK to commit. Comment Actions Oh, and thanks for working on this, this improvement was long overdue, but everybody was busy with something else :) Comment Actions I guess it is highly unlikely for someone to write namespaces like that, and if they do, I think they deserve to have them treated as a std::string. Comment Actions Thanks, Gábor! Comment Actions Yup. On the other hand, if we specify our method as {"basic_string", "c_str"} and they make a namespace basic_string { const char *c_str(); }, we are pretty likely to crash when we try to obtain this-value for the call that isn't a method. This is still not a big deal because it's still highly unlikely, but we've seen crash reports of this sort, and the easier our spec is, the more likely it becomes that somebody has a different thing with the same name. For example, if we want to model iterators and we specify {"begin"} without specifying the base class (so that all sorts of containers were covered), we still want to specify that we're looking for a method call and not for a global function call. So i believe that one of the important remaining problems with CallDescription is to teach it to discriminate between global functions and methods. We can do it in a number of ways:
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.