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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
