[analyzer] Refactor CallDescription match mode (NFC) #83432
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,20 +27,48 @@ class IdentifierInfo; | |
|
|
||
| namespace clang { | ||
| namespace ento { | ||
| /// A `CallDescription` is a pattern that can be used to _match_ calls | ||
| /// based on the qualified name and the argument/parameter counts. | ||
| class CallDescription { | ||
| public: | ||
| enum class Mode { | ||
| /// Match calls to functions from the C standard library. On some platforms | ||
| /// some functions may be implemented as macros that expand to calls to | ||
| /// built-in variants of the given functions, so in this mode we use some | ||
| /// heuristics to recognize these implementation-defined variants: | ||
| /// - We also accept calls where the name is derived from the specified | ||
| /// name by adding "__builtin" or similar prefixes/suffixes. | ||
| /// - We also accept calls where the number of arguments or parameters is | ||
| /// greater than the specified value. | ||
| /// For the exact heuristics, see CheckerContext::isCLibraryFunction(). | ||
| /// Note that functions whose declaration context is not a TU (e.g. | ||
| /// methods, functions in namespaces) are not accepted as C library | ||
| /// functions. | ||
| /// FIXME: If I understand it correctly, this discards calls where C++ code | ||
|
There was a problem hiding this comment. A FIXME comment should not be a documentation (only There was a problem hiding this comment. In general, I agree, but in this particular case I think this FIXME note is an important disclaimer and without it the auto-generated documentation would be misleading. |
||
| /// refers a C library function through the namespace `std::` via headers | ||
| /// like <cstdlib>. | ||
| CLibrary, | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm planning to ensure that:
This is an NFC commit so I didn't change the behavior of this mode yet, but I'm planning to do so in a followup commit. I picked the name Do you see any difficulties that would hinder or block this path? By the way I dislike the name There was a problem hiding this comment. It is good if it can be used for any C library (or like) function. I saw cases at MallocChecker where it is not used, then it should be used in these cases too. If for some reason we want to find C library functions that are not "builtin" another mode (the "Unspecified") can be used. Probably it is OK if later this mode can find functions in the There was a problem hiding this comment.
Now I realize that it would've been possible to resolve that bug by just adding After merging this commit, I'll quickly do a followup commit that fixes |
||
|
|
||
| /// Matches "simple" functions that are not methods. (Static methods are | ||
| /// methods.) | ||
| SimpleFunc, | ||
|
|
||
| /// Matches a C++ method (may be static, may be virtual, may be an | ||
| /// overloaded operator, a constructor or a destructor). | ||
| CXXMethod, | ||
|
|
||
| /// Match any CallEvent that is not an ObjCMethodCall. | ||
| /// FIXME: Previously this was the default behavior of CallDescription, but | ||
| /// its use should be replaced by a more specific mode almost everywhere. | ||
| Unspecified, | ||
|
There was a problem hiding this comment. This could have a better name like "Any" (or an other), "Unspecified" can mean that it works in a default way. There was a problem hiding this comment. At first I also wanted to name it "Any", but I discarded that idea because it doesn't match the Objective-C method calls. (And AFAIK Objective-C method calls are very weird stuff, whose AST/CallEvent representation is significantly different from the "normal" function/method calls, so I don't want to extend I'd say that reading There was a problem hiding this comment. My concern was really that we can think that "Unspecified" means one from the other modes is used. But this is likely not a big problem. There was a problem hiding this comment. I suggest that we go with the enumerator name 'Default', as the name of the enum is Mode, so it would be easily understood that this is something that comes out of the box. There was a problem hiding this comment. I picked the somewhat scary name "Unspecified" for the "match any kind of call" enumerator because it's too vague for practical use (if a checker wants to match something, it will practically always know if it's a function or a method) and I hope that I can eliminate its default-ness in follow-up commits. |
||
|
|
||
| /// FIXME: Add support for ObjCMethodCall events (I'm not adding it because | ||
| /// I'm not familiar with Objective-C). Note that currently an early return | ||
| /// in `bool matches(const CallEvent &Call) const;` discards all | ||
| /// Objective-C method calls. | ||
| }; | ||
|
|
||
| private: | ||
| friend class CallEvent; | ||
| using MaybeCount = std::optional<unsigned>; | ||
|
|
||
|
|
@@ -50,20 +78,26 @@ class CallDescription { | |
| std::vector<std::string> QualifiedName; | ||
| MaybeCount RequiredArgs; | ||
| MaybeCount RequiredParams; | ||
| Mode MatchAs; | ||
|
|
||
| public: | ||
| /// Constructs a CallDescription object. | ||
| /// | ||
| /// @param MatchAs Specifies the kind of the call that should be matched. | ||
| /// | ||
| /// @param QualifiedName The list of the name qualifiers of the function that | ||
| /// will be matched. The user is allowed to skip any of the qualifiers. | ||
| /// For example, {"std", "basic_string", "c_str"} would match both | ||
| /// std::basic_string<...>::c_str() and std::__1::basic_string<...>::c_str(). | ||
| /// | ||
| /// @param RequiredArgs The expected number of arguments that are passed to | ||
| /// the function. Omit this parameter (or pass std::nullopt) to match every | ||
| /// occurrence without checking the argument count in the call. | ||
| /// | ||
| /// @param RequiredParams The expected number of parameters in the function | ||
| /// definition that is called. Omit this parameter to match every occurrence | ||
| /// without checking the parameter count in the definition. | ||
| CallDescription(Mode MatchAs, ArrayRef<StringRef> QualifiedName, | ||
| MaybeCount RequiredArgs = std::nullopt, | ||
| MaybeCount RequiredParams = std::nullopt); | ||
|
|
||
|
|
@@ -222,6 +256,10 @@ template <typename T> class CallDescriptionMap { | |
| } | ||
| }; | ||
|
|
||
| /// Enumerators of this enum class are used to construct CallDescription | ||
| /// objects; in that context the fully qualified name is needlessly verbose. | ||
| using CDM = CallDescription::Mode; | ||
|
|
||
| /// An immutable set of CallDescriptions. | ||
| /// Checkers can efficiently decide if a given CallEvent matches any | ||
| /// CallDescription in the set. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should check how this looks in doxygen documentation (produces a list?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that it does, but I don't know how to generate the doxygen docs and I feel that messing with them is a waste of time.