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

David Blaikie via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Apr 14 01:13:30 PDT 2021


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:
> > > 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? 
> 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.
Oh. If this is user-facing then in must be stable/deterministic (for reproducibility, testing, etc).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100378



More information about the cfe-commits mailing list