[AST] Use IntrusiveRefCntPtr for Introspection LocationCall.

[njames93]

clang/lib/Tooling/NodeIntrospection.cpp

60

This is a slight change in behaviour, The old implementation used the operator< on a std::shared_ptr.
That in turns boils down to a comparison on the raw pointer, which is less than ideal.

[dblaikie]

clang/lib/Tooling/NodeIntrospection.cpp

60

Why is that less than ideal? Does the ordering need to be stable?

[njames93]

clang/lib/Tooling/NodeIntrospection.cpp

60

It's not strictly necessary that the ordering is stable. but when printing the contents of the map it would be nice if there was some obvious ordering. This also (partially) mimics the behaviour of the comparison for std::pair<SourceRange, SharedLocationCall>.

[dblaikie]

clang/lib/Tooling/NodeIntrospection.cpp

60

seems like a strange tradeoff to me for the LLVM codebase - we have lots of unstable maps & don't try to make them nicer for printing - especially if that would come at any performance cost, like doing string comparisons.

Does the SourceRange+SharedLocationCall need stability?

[njames93]

clang/lib/Tooling/NodeIntrospection.cpp

60

The purpose behind this functionality is partly based on getting clang-query to show how to get display what source location accessors can be used for specific nodes(http://ce.steveire.com/z/V3k6Rl). As this is for a user facing and not a particularly hot path, I think its worth a couple extra cycles to give stability.

[dblaikie]

clang/lib/Tooling/NodeIntrospection.cpp

60

Oh. If this is user-facing then in must be stable/deterministic (for reproducibility, testing, etc).

[steveire]

Indeed, I just this evening discovered the need for this to be stable across runs for testing.

clang/include/clang/Tooling/NodeIntrospection.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_NODEINTROSPECTION_H
 #define LLVM_CLANG_TOOLING_NODEINTROSPECTION_H
 
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/DeclarationName.h"
-
-#include <memory>
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include <set>
 
 namespace clang {
 
 class Stmt;
 class Decl;
 class CXXCtorInitializer;
 class NestedNameSpecifierLoc;
 class TemplateArgumentLoc;
 class CXXBaseSpecifier;
 
 namespace tooling {
 
-class LocationCall {
+class LocationCall;
+using SharedLocationCall = llvm::IntrusiveRefCntPtr<LocationCall>;
+
+class LocationCall : public llvm::ThreadSafeRefCountedBase<LocationCall> {
 public:
   enum LocationCallFlags { NoFlags, ReturnsPointer, IsCast };
-  LocationCall(std::shared_ptr<LocationCall> on, std::string name,
+  LocationCall(SharedLocationCall on, std::string name,
                LocationCallFlags flags = NoFlags)
-      : m_on(on), m_name(name), m_flags(flags) {}
-  LocationCall(std::shared_ptr<LocationCall> on, std::string name,
+      : m_flags(flags), m_on(std::move(on)), m_name(name) {}
+  LocationCall(SharedLocationCall on, std::string name,
                std::vector<std::string> const &args,
                LocationCallFlags flags = NoFlags)
-      : m_on(on), m_name(name), m_flags(flags) {}
+      : m_flags(flags), m_on(std::move(on)), m_name(name) {}
 
   LocationCall *on() const { return m_on.get(); }
   StringRef name() const { return m_name; }
   std::vector<std::string> const &args() const { return m_args; }
   bool returnsPointer() const { return m_flags & ReturnsPointer; }
   bool isCast() const { return m_flags & IsCast; }
 
 private:
-  std::shared_ptr<LocationCall> m_on;
+  LocationCallFlags m_flags;
+  SharedLocationCall m_on;
   std::string m_name;
   std::vector<std::string> m_args;
-  LocationCallFlags m_flags;
 };
 
 class LocationCallFormatterCpp {
 public:
   static std::string format(LocationCall *Call);
 };
 
 namespace internal {
 struct RangeLessThan {
-  bool operator()(
-      std::pair<SourceRange, std::shared_ptr<LocationCall>> const &LHS,
-      std::pair<SourceRange, std::shared_ptr<LocationCall>> const &RHS) const;
+  bool operator()(std::pair<SourceRange, SharedLocationCall> const &LHS,
+                  std::pair<SourceRange, SharedLocationCall> const &RHS) const;
+  bool
+  operator()(std::pair<SourceLocation, SharedLocationCall> const &LHS,
+             std::pair<SourceLocation, SharedLocationCall> const &RHS) const;
 };
+
 } // namespace internal
 
-template <typename T, typename U, typename Comp = std::less<std::pair<T, U>>>
-using UniqueMultiMap = std::set<std::pair<T, U>, Comp>;
+template <typename T, typename U>
+using UniqueMultiMap = std::set<std::pair<T, U>, internal::RangeLessThan>;
 
-using SourceLocationMap =
-    UniqueMultiMap<SourceLocation, std::shared_ptr<LocationCall>>;
-using SourceRangeMap =
-    UniqueMultiMap<SourceRange, std::shared_ptr<LocationCall>,
-                   internal::RangeLessThan>;
+using SourceLocationMap = UniqueMultiMap<SourceLocation, SharedLocationCall>;
+using SourceRangeMap = UniqueMultiMap<SourceRange, SharedLocationCall>;
 
 struct NodeLocationAccessors {
   SourceLocationMap LocationAccessors;
   SourceRangeMap RangeAccessors;
 };
 
 namespace NodeIntrospection {
 NodeLocationAccessors GetLocations(clang::Stmt const *Object);

clang/lib/Tooling/DumpTool/generate_cxx_src_locs.py

 
 using LocationAndString = SourceLocationMap::value_type;
 using RangeAndString = SourceRangeMap::value_type;
 """
 
     def GenerateBaseGetLocationsDeclaration(self, CladeName):
         self.implementationContent += \
             """
-void GetLocationsImpl(std::shared_ptr<LocationCall> const& Prefix,
+void GetLocationsImpl(SharedLocationCall const& Prefix,
     clang::{0} const *Object, SourceLocationMap &Locs,
     SourceRangeMap &Rngs);
 """.format(CladeName)
 
     def GenerateSrcLocMethod(self, ClassName, ClassData):
 
         self.implementationContent += \
             """
-static void GetLocations{0}(std::shared_ptr<LocationCall> const& Prefix,
+static void GetLocations{0}(SharedLocationCall const& Prefix,
     clang::{0} const &Object,
     SourceLocationMap &Locs, SourceRangeMap &Rngs)
 {{
 """.format(ClassName)
 
         if 'sourceLocations' in ClassData:
             for locName in ClassData['sourceLocations']:
                 self.implementationContent += \
                     """
   Locs.insert(LocationAndString(Object.{0}(),
-    std::make_shared<LocationCall>(Prefix, "{0}")));
+    llvm::makeIntrusiveRefCnt<LocationCall>(Prefix, "{0}")));
 """.format(locName)
 
             self.implementationContent += '\n'
 
         if 'sourceRanges' in ClassData:
             for rngName in ClassData['sourceRanges']:
                 self.implementationContent += \
                     """
   Rngs.insert(RangeAndString(Object.{0}(),
-    std::make_shared<LocationCall>(Prefix, "{0}")));
+    llvm::makeIntrusiveRefCnt<LocationCall>(Prefix, "{0}")));
 """.format(rngName)
 
             self.implementationContent += '\n'
 
         self.implementationContent += '}\n'
 
     def GenerateFiles(self, OutputFile):
         with open(os.path.join(os.getcwd(),
                   OutputFile), 'w') as f:
             f.write(self.implementationContent)
 
     def GenerateBaseGetLocationsFunction(self, ASTClassNames, CladeName):
 
         MethodReturnType = 'NodeLocationAccessors'
 
         Signature = \
             'GetLocations(clang::{0} const *Object)'.format(CladeName)
         ImplSignature = \
             """
-GetLocationsImpl(std::shared_ptr<LocationCall> const& Prefix,
+GetLocationsImpl(SharedLocationCall const& Prefix,
     clang::{0} const *Object, SourceLocationMap &Locs,
     SourceRangeMap &Rngs)
 """.format(CladeName)
 
         self.implementationContent += \
             'void {0} {{ GetLocations{1}(Prefix, *Object, Locs, Rngs);'.format(
                 ImplSignature,
                 CladeName)
 
         for ASTClassName in ASTClassNames:
             if ASTClassName != CladeName:
                 self.implementationContent += \
                     """
 if (auto Derived = llvm::dyn_cast<clang::{0}>(Object)) {{
   GetLocations{0}(Prefix, *Derived, Locs, Rngs);
 }}
 """.format(ASTClassName)
 
         self.implementationContent += '}'
 
         self.implementationContent += \
             """
 {0} NodeIntrospection::{1} {{
   NodeLocationAccessors Result;
-  std::shared_ptr<LocationCall> Prefix;
+  SharedLocationCall Prefix;
 
   GetLocationsImpl(Prefix, Object, Result.LocationAccessors,
                    Result.RangeAccessors);
 """.format(MethodReturnType,
                 Signature)
 
         self.implementationContent += 'return Result; }'
 

clang/lib/Tooling/NodeIntrospection.cpp

result +=
             .str();
   }
   result += (vec.back()->name() + "()").str();
   return result;
 }
 
 namespace internal {
 bool RangeLessThan::operator()(
-    std::pair<SourceRange, std::shared_ptr<LocationCall>> const &LHS,
-    std::pair<SourceRange, std::shared_ptr<LocationCall>> const &RHS) const {
+    std::pair<SourceRange, SharedLocationCall> const &LHS,
+    std::pair<SourceRange, SharedLocationCall> const &RHS) const {
   if (!LHS.first.isValid() || !RHS.first.isValid())
     return false;
 
   if (LHS.first.getBegin() < RHS.first.getBegin())
     return true;
   else if (LHS.first.getBegin() != RHS.first.getBegin())
     return false;
 
   if (LHS.first.getEnd() < RHS.first.getEnd())
     return true;
   else if (LHS.first.getEnd() != RHS.first.getEnd())
     return false;
 
   return LHS.second->name() < RHS.second->name();
 }
+bool RangeLessThan::operator()(
+    std::pair<SourceLocation, SharedLocationCall> const &LHS,
+    std::pair<SourceLocation, SharedLocationCall> const &RHS) const {
+  if (LHS.first == RHS.first)
+    return LHS.second->name() < RHS.second->name();
+  return LHS.first < RHS.first;
+}
 } // namespace internal
 
 } // namespace tooling
 } // namespace clang
 
 #include "clang/Tooling/NodeIntrospection.inc"