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