[PATCH] D100638: [AST][Introspection] Avoid creating temporary strings when comparing LocationCalls.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.


================
Comment at: clang/lib/Tooling/NodeIntrospection.cpp:84
+
+  auto LI = Left.rbegin(), LE = Left.rend(), RI = Right.rbegin();
+  for (; LI != LE; ++LI, ++RI) {
----------------
njames93 wrote:
> steveire wrote:
> > Would it make sense to compare the sizes `(leftsize < rightsize) return true` etc and only loop if the containers are the same size?
> That wouldn't work as we are comparing lexicographically. If leftsize was smaller than rightsize, but the first item in left was `zzzz`, and first in right was `aaaa`. We'd want right to be marked as less then left.
I don't think we would want that. All we need is to be able to put these things in a std set. We only need uniquenes and deterministic order. We don't need or want any particular order at all. 

Besides, your implementation orders elements as they are formatted for c++, which could differ from the order the calls would be in if casts are omitted (js, python).

Besides, https://en.m.wikipedia.org/wiki/Lexicographic_order "One variant applies to sequences of different lengths by comparing the lengths of the sequences before considering their elements."

It should be possible to implement this without creating the local vectors. Just iterate in parallel through the location calls, comparing names, and if one chain ends before the other return early. That would seem to have all the properties we need, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100638