[PATCH] D125863: [clangd] Dont mark terminating PP-directives as skipped

Wed May 18 05:10:55 PDT 2022

hokein added a comment.

sorry, I might be lack of the context, where is the user complaint? I'm not sure which cases are improved with this patch.

Ideally we would not mark PP directives as inactive regions, but we never do that (FIXME <https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/SemanticHighlighting.cpp#L443>), I think we're trying to fix that?

================
Comment at: clang-tools-extra/clangd/CollectMacros.h:90
+    // Don't mark the terminating PP-directive as skipped.
+    End.character = 0;
     Out.SkippedRanges.push_back(Range{Begin, End});
----------------
This looks like a  semantic-highlight-specific change, instead of doing it here, would it make more sense to do it in the `SemanticHighlighting.cpp`?

================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:447
 $InactiveCode[[#if defined(test)]]
-$InactiveCode[[#endif]]
+#endif
     )cpp",
----------------
It seems to me that the new behavior of these cases is worse --  `#ifedf` will not be highlighted while the paired `#endif` will,  this inconsistency probably gives weird and confusing UI experience to users. 

I think it is important to have a consistent decision -- highlight both or not-highlight both.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125863