[analyzer][NFC] Consolidate the inner representation of CallDescriptions

[steakhal]

Summary

CallDescriptions have a RequiredArgs and RequiredParams members,
but they are of different types, unsigned and size_t respectively.
In the patch I use only unsigned for both, that should be large enough
anyway.

In the patch, I also avoid the use of the smart less-than operator.

template <typename T>
constexpr bool operator<=(const Optional<T> &X, const T &Y);

Which would check if the optional has a value and compare the data
only after. I found it surprising, thus I think we are better off
without it.

[steakhal]

rebase

[xazax.hun]

Nice cleanup!

[xazax.hun]

While you have a large stack, some of these changes should be relatively independent. I'd recommend committing those regardless of the status of the rest of the stack.

[steakhal]

In D113594#3127756, @xazax.hun wrote:

While you have a large stack, some of these changes should be relatively independent. I'd recommend committing those regardless of the status of the rest of the stack.

Using linear history is way more convenient for me.
I will split the stack if we don't reach a consensus on the stack though.

[martong]

In the patch, I also avoid the use of the smart less-than operator.

It's not obvious for me how you avoid it. Could you please elaborate?

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h

48–50

What about using a common type alias for these two?

[steakhal]

In D113594#3140568, @martong wrote:

In the patch, I also avoid the use of the smart less-than operator.

It's not obvious for me how you avoid it. Could you please elaborate?

You can see in the diff that in some cases I'm using the dereference operator of the Optional in the new code, whereas previously we did not.
It was because the operator== and operator<= did the nullness check of the Optional within their operator implementation.
As you can see, one does not regularly see such a thing, so we are better off with that IMO.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h

48–50

We could have some alias type, but I would rather choose MaybeCount or MaybeInt.
Which would you prefer?

[martong]

In D113594#3140939, @steakhal wrote:

In D113594#3140568, @martong wrote:

In the patch, I also avoid the use of the smart less-than operator.

It's not obvious for me how you avoid it. Could you please elaborate?

You can see in the diff that in some cases I'm using the dereference operator of the Optional in the new code, whereas previously we did not.
It was because the operator== and operator<= did the nullness check of the Optional within their operator implementation.
As you can see, one does not regularly see such a thing, so we are better off with that IMO.

Okay, thanks, makes sense.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h

48–50

I prefer MaybeCount, thanks for asking.

[steakhal]

• introduce the MaybeUInt type alias

[steakhal]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h

48–50

What about MaybeUInt? On second thought, MaybeCount might be even better.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h

enum CallDescriptionFlags : int {
   /// number of arguments.
   CDF_MaybeBuiltin = 1 << 0,
 };
 
 /// This class represents a description of a function call using the number of
 /// arguments and the name of the function.
 class CallDescription {
   friend class CallEvent;
+  using MaybeUInt = Optional<unsigned>;
+
   mutable Optional<const IdentifierInfo *> II;
   // The list of the qualified names used to identify the specified CallEvent,
   // e.g. "{a, b}" represent the qualified names, like "a::b".
   std::vector<std::string> QualifiedName;
-  Optional<unsigned> RequiredArgs;
-  Optional<size_t> RequiredParams;
+  MaybeUInt RequiredArgs;
+  MaybeUInt RequiredParams;
   int Flags;
 
 public:
   /// Constructs a CallDescription object.
   ///
   /// @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 number of arguments that is expected to match a
   /// call. Omit this parameter to match every occurrence of call with a given
   /// name regardless the number of arguments.
   CallDescription(int Flags, ArrayRef<const char *> QualifiedName,
-                  Optional<unsigned> RequiredArgs = None,
-                  Optional<size_t> RequiredParams = None);
+                  MaybeUInt RequiredArgs = None,
+                  MaybeUInt RequiredParams = None);
 
   /// Construct a CallDescription with default flags.
   CallDescription(ArrayRef<const char *> QualifiedName,
-                  Optional<unsigned> RequiredArgs = None,
-                  Optional<size_t> RequiredParams = None);
+                  MaybeUInt RequiredArgs = None,
+                  MaybeUInt RequiredParams = None);
 
   CallDescription(std::nullptr_t) = delete;
 
   /// Get the name of the function that this object matches.
   StringRef getFunctionName() const { return QualifiedName.back(); }
 
   /// Get the qualified name parts in reversed order.
   /// E.g. { "std", "vector", "data" } -> "vector", "std"

clang/lib/StaticAnalyzer/Core/CallDescription.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include <iterator>
 
 using namespace llvm;
 using namespace clang;
 
+using MaybeUInt = Optional<unsigned>;
+
 // A constructor helper.
-static Optional<size_t> readRequiredParams(Optional<unsigned> RequiredArgs,
-                                           Optional<size_t> RequiredParams) {
+static MaybeUInt readRequiredParams(MaybeUInt RequiredArgs,
+                                    MaybeUInt RequiredParams) {
   if (RequiredParams)
     return RequiredParams;
   if (RequiredArgs)
-    return static_cast<size_t>(*RequiredArgs);
+    return RequiredArgs;
   return None;
 }
 
-ento::CallDescription::CallDescription(
-    int Flags, ArrayRef<const char *> QualifiedName,
-    Optional<unsigned> RequiredArgs /*= None*/,
-    Optional<size_t> RequiredParams /*= None*/)
+ento::CallDescription::CallDescription(int Flags,
+                                       ArrayRef<const char *> QualifiedName,
+                                       MaybeUInt RequiredArgs /*= None*/,
+                                       MaybeUInt RequiredParams /*= None*/)
     : RequiredArgs(RequiredArgs),
       RequiredParams(readRequiredParams(RequiredArgs, RequiredParams)),
       Flags(Flags) {
   assert(!QualifiedName.empty());
   this->QualifiedName.reserve(QualifiedName.size());
   llvm::copy(QualifiedName, std::back_inserter(this->QualifiedName));
 }
 
 /// Construct a CallDescription with default flags.
-ento::CallDescription::CallDescription(
-    ArrayRef<const char *> QualifiedName,
-    Optional<unsigned> RequiredArgs /*= None*/,
-    Optional<size_t> RequiredParams /*= None*/)
+ento::CallDescription::CallDescription(ArrayRef<const char *> QualifiedName,
+                                       MaybeUInt RequiredArgs /*= None*/,
+                                       MaybeUInt RequiredParams /*= None*/)
     : CallDescription(0, QualifiedName, RequiredArgs, RequiredParams) {}
 
 bool ento::CallDescription::matches(const CallEvent &Call) const {
   // FIXME: Add ObjC Message support.
   if (Call.getKind() == CE_ObjCMessage)
     return false;
 
   const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
   if (!FD)
     return false;
 
   if (Flags & CDF_MaybeBuiltin) {
     return CheckerContext::isCLibraryFunction(FD, getFunctionName()) &&
-           (!RequiredArgs || RequiredArgs <= Call.getNumArgs()) &&
-           (!RequiredParams || RequiredParams <= Call.parameters().size());
+           (!RequiredArgs || *RequiredArgs <= Call.getNumArgs()) &&
+           (!RequiredParams || *RequiredParams <= Call.parameters().size());
   }
 
   if (!II.hasValue()) {
     II = &Call.getState()->getStateManager().getContext().Idents.get(
         getFunctionName());
   }
 
   const auto MatchNameOnly = [](const CallDescription &CD,
                                 const NamedDecl *ND) -> bool {
     DeclarationName Name = ND->getDeclName();
     if (const auto *II = Name.getAsIdentifierInfo())
       return II == CD.II.getValue(); // Fast case.
 
     // Fallback to the slow stringification and comparison for:
     // C++ overloaded operators, constructors, destructors, etc.
     // FIXME This comparison is way SLOWER than comparing pointers.
     // At some point in the future, we should compare FunctionDecl pointers.
     return Name.getAsString() == CD.getFunctionName();
   };
 
   const auto ExactMatchArgAndParamCounts =
       [](const CallEvent &Call, const CallDescription &CD) -> bool {
     const bool ArgsMatch =
-        !CD.RequiredArgs || CD.RequiredArgs == Call.getNumArgs();
+        !CD.RequiredArgs || *CD.RequiredArgs == Call.getNumArgs();
     const bool ParamsMatch =
-        !CD.RequiredParams || CD.RequiredParams == Call.parameters().size();
+        !CD.RequiredParams || *CD.RequiredParams == Call.parameters().size();
     return ArgsMatch && ParamsMatch;
   };
 
   const auto MatchQualifiedNameParts = [](const CallDescription &CD,
                                           const Decl *D) -> bool {
     const auto FindNextNamespaceOrRecord =
         [](const DeclContext *Ctx) -> const DeclContext * {
       while (Ctx && !isa<NamespaceDecl, RecordDecl>(Ctx))