[PATCH] D100378: [AST] Use IntrusiveRefCntPtr for Introspection LocationCall.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Tooling/NodeIntrospection.cpp:60
+  if (LHS.first == RHS.first)
+    return LHS.second->name() < RHS.second->name();
+  return LHS.first < RHS.first;
----------------
njames93 wrote:
> dblaikie wrote:
> > njames93 wrote:
> > > 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.
> > Why is that less than ideal? Does the ordering need to be stable?
> 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>`.
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? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100378/new/

https://reviews.llvm.org/D100378