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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
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:
> 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) 
Oh, okay, I thought `tryGetRealPathName` would not be empty here :(

The problem with `getName` is that it turned out to produce the same results that we already get right now through SourceManager.

That leaves us with other ways of canonicalization: either through conversions to the native path or `FileManager::getCanonicalName()`. Both are expensive and I think the problem is that though the mapping here would be OK to generate in an expensive manner, the callsites would not be very happy with that but I guess it's acceptable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123031