[clang][Basic] Integrate SourceLocation with FoldingSet, NFCI

[miyuki]

Summary

This patch removes the necessity to access the SourceLocation internal
representation in several places that use FoldingSet objects.

[miyuki]

Rebased, added similar integration for SourceRange

[dexonsmith]

clang/include/clang/Basic/SourceLocation.h

183–184

Seems a bit strange to me to add SourceLocation::Profile. Is there another hook available for FoldingSet that works with free functions, or template specializations? You could make that hook a friend in order to give it access to the internals. Or can the caller just use getHashValue()?

243–245

I'm not sure you need this change since you don't need to give access to internals here.

[miyuki]

Addressed review comments

[miyuki]

Reverted the accidentally removed chunk.

[miyuki]

Fixed linter errors.

[dexonsmith]

clang/lib/Analysis/PathDiagnostic.cpp

1088

I'm surprised you need this static_cast, can you explain why it was necessary?

[miyuki]

clang/lib/Analysis/PathDiagnostic.cpp

1088

Loc is an object of type FullSourceLoc, a class derived from SourceLocation. When the FoldingSetNodeID::Add template method is called with Loc it tries to instantiate FoldingSetTrait<FullSourceLoc> and fails (because there is no explicit specialization for FullSourceLoc and the default one relies on the method FullSourceLoc::Profile which does not exist). FullSourceLoc does not contain any additional information about the location itself (it just holds a pointer to the associated SourceManager), so there is no need to specialize FoldingSetTrait for it, and casting to SourceLocation will make FoldingSetNodeID::Add use the correct FoldingSetTrait specialization.

[dexonsmith]

LGTM.

clang/lib/Analysis/PathDiagnostic.cpp

1088

Right, that makes sense. Thanks for explaining!

clang/include/clang/Basic/SourceLocation.h

 #include <cstdint>
 #include <string>
 #include <utility>
 
 namespace llvm {
 
 template <typename T> struct DenseMapInfo;
 
+class FoldingSetNodeID;
+template <typename T> struct FoldingSetTrait;
+
 } // namespace llvm
 
 namespace clang {
 
 class SourceManager;
 
 /// An opaque identifier used by SourceManager which refers to a
 /// source file (MemoryBuffer) along with its \#include path and \#line data.

 /// In addition, one bit of SourceLocation is used for quick access to the
 /// information whether the location is in a file or a macro expansion.
 ///
 /// It is important that this type remains small. It is currently 32 bits wide.
 class SourceLocation {
   friend class ASTReader;
   friend class ASTWriter;
   friend class SourceManager;
+  friend struct llvm::FoldingSetTrait<SourceLocation>;
 
   unsigned ID = 0;
 
   enum : unsigned {
     MacroIDBit = 1U << 31
   };
 
 public:
public:
   }
 
   static bool isPairOfFileLocations(SourceLocation Start, SourceLocation End) {
     return Start.isValid() && Start.isFileID() && End.isValid() &&
            End.isFileID();
   }
 
   unsigned getHashValue() const;
   void print(raw_ostream &OS, const SourceManager &SM) const;
   std::string printToString(const SourceManager &SM) const;
   void dump(const SourceManager &SM) const;
 };
 
 inline bool operator==(const SourceLocation &LHS, const SourceLocation &RHS) {
   return LHS.getRawEncoding() == RHS.getRawEncoding();
 }
 
 inline bool operator!=(const SourceLocation &LHS, const SourceLocation &RHS) {
bool operator!=(const SourceRange &X) const {
     return B != X.B || E != X.E;
   }
 
   // Returns true iff other is wholly contained within this range.
   bool fullyContains(const SourceRange &other) const {
     return B <= other.B && E >= other.E;
   }
 
   void print(raw_ostream &OS, const SourceManager &SM) const;
   std::string printToString(const SourceManager &SM) const;
   void dump(const SourceManager &SM) const;
 };
 
 /// Represents a character-granular source range.
 ///
 /// The underlying SourceRange can either specify the starting/ending character
 /// of the range, or it can specify the start of the range and the start of the
 /// last token of the range (a "token range").  In the token range case, the
 /// size of the last token must be measured to determine the actual end of the
static unsigned getHashValue(clang::SourceLocation Loc) {
       return Loc.getHashValue();
     }
 
     static bool isEqual(clang::SourceLocation LHS, clang::SourceLocation RHS) {
       return LHS == RHS;
     }
   };
 
+  // Allow calling FoldingSetNodeID::Add with SourceLocation object as parameter
+  template <> struct FoldingSetTrait<clang::SourceLocation> {
+    static void Profile(const clang::SourceLocation &X, FoldingSetNodeID &ID);
+  };
+
   // Teach SmallPtrSet how to handle SourceLocation.
   template<>
   struct PointerLikeTypeTraits<clang::SourceLocation> {
     static constexpr int NumLowBitsAvailable = 0;
 
     static void *getAsVoidPointer(clang::SourceLocation L) {
       return L.getPtrEncoding();
     }

clang/lib/Analysis/PathDiagnostic.cpp

unsigned PathDiagnostic::full_size() {
   return size;
 }
 
 //===----------------------------------------------------------------------===//
 // FoldingSet profiling methods.
 //===----------------------------------------------------------------------===//
 
 void PathDiagnosticLocation::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger(Range.getBegin().getRawEncoding());
-  ID.AddInteger(Range.getEnd().getRawEncoding());
-  ID.AddInteger(Loc.getRawEncoding());
+  ID.Add(Range.getBegin());
+  ID.Add(Range.getEnd());
+  ID.Add(static_cast<const SourceLocation &>(Loc));
 }
 
 void PathDiagnosticPiece::Profile(llvm::FoldingSetNodeID &ID) const {
   ID.AddInteger((unsigned) getKind());
   ID.AddString(str);
   // FIXME: Add profiling support for code hints.
   ID.AddInteger((unsigned) getDisplayHint());
   ArrayRef<SourceRange> Ranges = getRanges();
   for (const auto &I : Ranges) {
-    ID.AddInteger(I.getBegin().getRawEncoding());
-    ID.AddInteger(I.getEnd().getRawEncoding());
+    ID.Add(I.getBegin());
+    ID.Add(I.getEnd());
   }
 }
 
 void PathDiagnosticCallPiece::Profile(llvm::FoldingSetNodeID &ID) const {
   PathDiagnosticPiece::Profile(ID);
   for (const auto &I : path)
     ID.Add(*I);
 }

clang/lib/Basic/SourceLocation.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/DenseMapInfo.h"
+#include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <string>
 #include <utility>
 

 //===----------------------------------------------------------------------===//
 // SourceLocation
 //===----------------------------------------------------------------------===//
 
 unsigned SourceLocation::getHashValue() const {
   return llvm::DenseMapInfo<unsigned>::getHashValue(ID);
 }
 
+void llvm::FoldingSetTrait<SourceLocation>::Profile(
+    const SourceLocation &X, llvm::FoldingSetNodeID &ID) {
+  ID.AddInteger(X.ID);
+}
+
 void SourceLocation::print(raw_ostream &OS, const SourceManager &SM)const{
   if (!isValid()) {
     OS << "<invalid loc>";
     return;
   }
 
   if (isFileID()) {
     PresumedLoc PLoc = SM.getPresumedLoc(*this);

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

void BasicBugReport::Profile(llvm::FoldingSetNodeID& hash) const {
   hash.AddPointer(&BT);
   hash.AddString(Description);
   assert(Location.isValid());
   Location.Profile(hash);
 
   for (SourceRange range : Ranges) {
     if (!range.isValid())
       continue;
-    hash.AddInteger(range.getBegin().getRawEncoding());
-    hash.AddInteger(range.getEnd().getRawEncoding());
+    hash.Add(range.getBegin());
+    hash.Add(range.getEnd());
   }
 }
 
 void PathSensitiveBugReport::Profile(llvm::FoldingSetNodeID &hash) const {
   hash.AddInteger(static_cast<int>(getKind()));
   hash.AddPointer(&BT);
   hash.AddString(Description);
   PathDiagnosticLocation UL = getUniqueingLocation();
   if (UL.isValid()) {
     UL.Profile(hash);
   } else {
     // TODO: The statement may be null if the report was emitted before any
     // statements were executed. In particular, some checkers by design
     // occasionally emit their reports in empty functions (that have no
     // statements in their body). Do we profile correctly in this case?
     hash.AddPointer(ErrorNode->getCurrentOrPreviousStmtForDiagnostics());
   }
 
   for (SourceRange range : Ranges) {
     if (!range.isValid())
       continue;
-    hash.AddInteger(range.getBegin().getRawEncoding());
-    hash.AddInteger(range.getEnd().getRawEncoding());
+    hash.Add(range.getBegin());
+    hash.Add(range.getEnd());
   }
 }
 
 template <class T>
 static void insertToInterestingnessMap(
     llvm::DenseMap<T, bugreporter::TrackingKind> &InterestingnessMap, T Val,
     bugreporter::TrackingKind TKind) {
   auto Result = InterestingnessMap.insert({Val, TKind});