[PATCH] D78740: [clangd] Handle PresumedLocations in IncludeCollector

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Headers.cpp:39
                           SrcMgr::CharacteristicKind FileKind) override {
-    if (isInsideMainFile(HashLoc, SM)) {
+    auto AddMainFileInc = [&](SourceLocation HashLoc) {
       Out->MainFileIncludes.emplace_back();
----------------
sammccall wrote:
> sammccall wrote:
> > nit: Inc -> Include
> this structure is not terrible but the reverse-chronological-order hurts readability a bit.
> 
> You could try:
> ```
> // If an include is part of the preamble patch, translate #line directives.
> if (BuiltinFileInStack) {
>   auto PreLoc = ...;
>   if (...) {
>     HashLoc = ...; // now we'll hit the case below
>   }
> }
> // Record main-file inclusions (including those mapped from the preamble patch).
> if (isInsideMainFile(HashLoc, SM)) {
>   // add the include
> }
> ```
well this was the initial version :D moved away from it since you asked for an explicit function both cases can call, reverting back to it.


================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:314
+  HeaderContents += R"cpp(
+#line 1
+#include <a.h>)cpp";
----------------
sammccall wrote:
> Can we use more aribtrary placeholder here, like 42? :)
> We could easily hit 1 by mistake.
unfortunately we can't, at least not without having that line available in the main file. Since we need to get a `real` offset into it in the end.

changing it to be line 3 as a middle ground :P


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78740