[PATCH] D43823: [clangd] Prefer the definition of a TagDecl (e.g. class) as CanonicalDeclaration.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/index/FileIndex.cpp:23
   SymbolCollector::Options CollectorOpts;
   // Although we do not index symbols in main files (e.g. cpp file), information
   // in main files like definition locations of class declarations will still be
----------------
this comment doesn't make sense here anymore. I think it's ok just to remove, otherwise add to the documentation of symbolcollector I guess.


================
Comment at: clangd/index/SymbolCollector.cpp:260
       BasicSymbol = addDeclaration(*ND, std::move(ID));
+    else if (isPreferredDeclaration(OriginalDecl, Roles))
+      BasicSymbol = addDeclaration(OriginalDecl, std::move(ID));
----------------
This is a bit subtle and I think comment-worthy.
We're doing a bunch of duplicated work, but at most once because we expect to see only one preferred declaration per TU, because in practice they're definitions?

An alternative would be to have addDeclaration loop through the redecls to try to find a preferred one (and use the passed in one if none is found). This would avoid any duplicated work. Up to you.


================
Comment at: clangd/index/SymbolCollector.cpp:289
   std::string FileURI;
   // FIXME: we may want a different "canonical" heuristic than clang chooses.
   // Clang seems to choose the first, which may not have the most information.
----------------
You're addressing this fixme. (I expected it to be done inside addDeclaration, which is why the fixme is here - may still be a good idea as noted above).


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:218
-
-    // Declared/defined in main only.
-    int $y[[Y]];
----------------
please keep this and verify it's not indexed.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:222
                 DefRange(Main.offsetRange("xdef"))),
-          AllOf(QName("Cls"), DeclRange(Header.offsetRange("clsdecl")),
+          AllOf(QName("Cls"), DeclURI(TestHeaderURI),
+                DeclRange(Header.offsetRange("clsdecl")),
----------------
i'm not sure it's necessary to assert the file - it's inconsistent (i don't think there's anything special about this one?) and would add a lot of noise to do consistently.

There's not much risk of an actual collision, right? up to you though


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43823