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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
