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

Kirill Bobyrev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 4 08:20:47 PDT 2022

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list