[PATCH] D117549: [clangd] Sort targets before printing for tests

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: usaxena95, arphaman, mgrang.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Targets are not necessarily inserted in the order they appear in source
code. For example we could traverse overload sets, or selectively insert
template patterns after all other decls.
So order the targets before printing to make sure tests are not dependent on
such implementation details. We can also do it in production, but that might be
wasteful as we haven't seen any complaints in the wild around these orderings
yet.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117549

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp


Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -1284,10 +1284,6 @@
         "1: targets = {vector}\n"
         "2: targets = {x}\n"},
 // Handle UnresolvedLookupExpr.
-// FIXME
-// This case fails when expensive checks are enabled.
-// Seems like the order of ns1::func and ns2::func isn't defined.
-#ifndef EXPENSIVE_CHECKS
        {R"cpp(
             namespace ns1 { void func(char*); }
             namespace ns2 { void func(int*); }
@@ -1301,7 +1297,6 @@
         )cpp",
         "0: targets = {ns1::func, ns2::func}\n"
         "1: targets = {t}\n"},
-#endif
        // Handle UnresolvedMemberExpr.
        {R"cpp(
             struct X {
Index: clang-tools-extra/clangd/FindTarget.cpp
===================================================================
--- clang-tools-extra/clangd/FindTarget.cpp
+++ clang-tools-extra/clangd/FindTarget.cpp
@@ -33,6 +33,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
@@ -1170,6 +1171,13 @@
   // note we cannot print R.NameLoc without a source manager.
   OS << "targets = {";
   bool First = true;
+  if (!R.Targets.empty()) {
+    const auto &SM = R.Targets.front()->getASTContext().getSourceManager();
+    llvm::sort(R.Targets, [&SM](const NamedDecl *LHS, const NamedDecl *RHS) {
+      return SM.isBeforeInTranslationUnit(LHS->getLocation(),
+                                          RHS->getLocation());
+    });
+  }
   for (const NamedDecl *T : R.Targets) {
     if (!First)
       OS << ", ";



Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -1284,10 +1284,6 @@
         "1: targets = {vector}\n"
         "2: targets = {x}\n"},
 // Handle UnresolvedLookupExpr.
-// FIXME
-// This case fails when expensive checks are enabled.
-// Seems like the order of ns1::func and ns2::func isn't defined.
-#ifndef EXPENSIVE_CHECKS
        {R"cpp(
             namespace ns1 { void func(char*); }
             namespace ns2 { void func(int*); }
@@ -1301,7 +1297,6 @@
         )cpp",
         "0: targets = {ns1::func, ns2::func}\n"
         "1: targets = {t}\n"},
-#endif
        // Handle UnresolvedMemberExpr.
        {R"cpp(
             struct X {
Index: clang-tools-extra/clangd/FindTarget.cpp
===================================================================
--- clang-tools-extra/clangd/FindTarget.cpp
+++ clang-tools-extra/clangd/FindTarget.cpp
@@ -33,6 +33,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
@@ -1170,6 +1171,13 @@
   // note we cannot print R.NameLoc without a source manager.
   OS << "targets = {";
   bool First = true;
+  if (!R.Targets.empty()) {
+    const auto &SM = R.Targets.front()->getASTContext().getSourceManager();
+    llvm::sort(R.Targets, [&SM](const NamedDecl *LHS, const NamedDecl *RHS) {
+      return SM.isBeforeInTranslationUnit(LHS->getLocation(),
+                                          RHS->getLocation());
+    });
+  }
   for (const NamedDecl *T : R.Targets) {
     if (!First)
       OS << ", ";

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits