[PATCH] D108045: [clangd] Fix clangd crash when including a header

Mon Aug 23 01:06:13 PDT 2021

kadircet added a comment.

Thanks, I can see the problem now (sorry for the late reply, i was on leave last week).

It is amazing that this hasn't bitten us yet during code complete flow (it probably did, but clangd would recover after restart so probably people didn't notice).
I still don't think that we'd like to parse all the new includes during code completion flow hence we should just create a preamble patch for the macro directives that moved around. (I don't think we should make the parser more resilient instead).
Do you mind adding a new factory function to PreamblePatch for creating a macro-directive only patch that can be used in CodeComplete flow to prevent crashes (while paying for some extra latency :/  )?

As for storing source locations, i think we can just store the offsets (we already have them in `spellDirective` when we decompose sourcelocation.

Does that make sense?

================
Comment at: clang-tools-extra/clangd/Preamble.cpp:511

   if (DirectivesChanged) {
     // We need to patch all the directives, since they are order dependent. e.g:
----------------
this is the condition i am suggesting to drop.

================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:553
+  llvm::StringLiteral Modified =
+      "#include \"header.h\"\n#define MACRO 12\nint num = MACRO;";
+  auto AST = createPatchedAST(Baseline, Modified);
----------------
we actually have a more problematic case, you can just insert whitespace to the first line rather than the `include` directive. now all the offsets are wrong but there's nothing to patch.

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

https://reviews.llvm.org/D108045