[PATCH] D123031: [clangd] Use consistent header paths in CanonicalIncludes mappings

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Changing the strings here makes sense, but I'm confused by the use of tryGetRealPathName.
Also, we ended up dropping the filename in favor of UniqueID in IncludeStructure, so that's an alternative here.

Yes, I think we need a test here with a scope wide enough that it tests that the callsites of addMapping/getMapping line up.



================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:73
+      auto Filename = SM.getFileEntryForID(SM.getFileID(Range.getBegin()))
+                          ->tryGetRealPathName();
+      Includes->addMapping(Filename, isLiteralInclude(Text)
----------------
offline we had discussed getName rather than tryGetRealPathName instead.

The comment in HeaderStructure says specifically that we avoid tryGetRealPathName because it's not preserved in the preamble.
I suspect that this will give incorrect results in cases involving non-preamble `#includes`.

In any case we should have a comment somewhere on CanonicalIncludes about how we're identifying files, and why it's stable enough. The lack of such a comment is really what led to the bug on friday :-)


================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:73
+      auto Filename = SM.getFileEntryForID(SM.getFileID(Range.getBegin()))
+                          ->tryGetRealPathName();
+      Includes->addMapping(Filename, isLiteralInclude(Text)
----------------
sammccall wrote:
> offline we had discussed getName rather than tryGetRealPathName instead.
> 
> The comment in HeaderStructure says specifically that we avoid tryGetRealPathName because it's not preserved in the preamble.
> I suspect that this will give incorrect results in cases involving non-preamble `#includes`.
> 
> In any case we should have a comment somewhere on CanonicalIncludes about how we're identifying files, and why it's stable enough. The lack of such a comment is really what led to the bug on friday :-)
tryGetRealPathName can be the empty string, what do we do in such cases?

(In practice I think this is going to happen in the case where you have a non-preamble `#include` of a header-guarded file which was also part of the preamble) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123031