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

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Your implementation is getting very complicated and it requires many comments.

Also, if we get to this point in the execution of `RangeLessThan::operator()`, you're creating and populating two vectors for every two elements contained. I know it's `llvm::SmallVector` and maybe not so expensive etc, but doing something is more expensive than not doing it.

You're trying to sort elements (which have the same location/range) alphabetically according to how it will be sorted after c++ formatting. Sorting according to c++ formatting means sorting incorrectly according to JS/python. This is simple to demonstrate:

  TEST(Introspection, SourceLocations_CallContainer3) {
  
    SourceLocationMap slm;
    SharedLocationCall Prefix;
    auto call1 = llvm::makeIntrusiveRefCnt<LocationCall>(
                                          llvm::makeIntrusiveRefCnt<LocationCall>(
                                          llvm::makeIntrusiveRefCnt<LocationCall>(
                                          Prefix, "dddd"), "eeee", LocationCall::IsCast), "ffff");
    auto call2 = llvm::makeIntrusiveRefCnt<LocationCall>(
                                          llvm::makeIntrusiveRefCnt<LocationCall>(
                                          llvm::makeIntrusiveRefCnt<LocationCall>(
                                          Prefix, "dddd"), "gggg", LocationCall::IsCast), "bbbb");
    slm.insert(
        std::make_pair(SourceLocation(), call1));
    slm.insert(
        std::make_pair(SourceLocation(), call2));
  
    for (auto& item : slm)
    {
      llvm::errs() << LocationCallFormatterCpp::format(*item.second) << "\n";
    }
    for (auto& item : slm)
    {
      llvm::errs() << LocationCallFormatterSimple::format(*item.second) << "\n";
    }
  }

output:

  dddd().eeee().ffff()
  dddd().gggg().bbbb()
  dddd().ffff()
  dddd().bbbb()

So, the cost of creating two vectors for each invocation of `lessThan` that reaches this point delivers no benefit.

Here is an implementation which does not create those vectors:

  diff --git a/clang/lib/Tooling/NodeIntrospection.cpp b/clang/lib/Tooling/NodeIntrospection.cpp
  index 8e2d446c3efe..6e846acf9ecf 100644
  --- a/clang/lib/Tooling/NodeIntrospection.cpp
  +++ b/clang/lib/Tooling/NodeIntrospection.cpp
  @@ -40,6 +40,22 @@ std::string LocationCallFormatterCpp::format(const LocationCall &Call) {
     return Result;
   }
   
  +bool locationLessThan(const LocationCall *LHS, const LocationCall *RHS)
  +{
  +  if (!LHS && !RHS)
  +    return false;
  +  if (LHS && !RHS)
  +    return true;
  +  if (!LHS && RHS)
  +    return false;
  +  auto compareResult = LHS->name().compare(RHS->name());
  +  if (compareResult < 0)
  +    return true;
  +  if (compareResult > 0)
  +    return false;
  +  return locationLessThan(LHS->on(), RHS->on());
  +}
  +
   namespace internal {
   bool RangeLessThan::operator()(
       std::pair<SourceRange, SharedLocationCall> const &LHS,
  @@ -54,15 +70,13 @@ bool RangeLessThan::operator()(
     else if (LHS.first.getEnd() != RHS.first.getEnd())
       return false;
   
  -  return LocationCallFormatterCpp::format(*LHS.second) <
  -         LocationCallFormatterCpp::format(*RHS.second);
  +  return locationLessThan(LHS.second.get(), RHS.second.get());
   }
   bool RangeLessThan::operator()(
       std::pair<SourceLocation, SharedLocationCall> const &LHS,
       std::pair<SourceLocation, SharedLocationCall> const &RHS) const {
     if (LHS.first == RHS.first)
  -    return LocationCallFormatterCpp::format(*LHS.second) <
  -           LocationCallFormatterCpp::format(*RHS.second);
  +    return locationLessThan(LHS.second.get(), RHS.second.get());
     return LHS.first < RHS.first;
   }
   } // namespace internal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100638