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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added a comment.

In D125863#3522025 <https://reviews.llvm.org/D125863#3522025>, @hokein wrote:

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

so the complaint was from an example with an active branch, eg:

  #if 1
  void foo();
  #else
  #endif

The user was complaining that `#endif` was inactive depending on first or second branch being active. Hence my initial thoughts were also around cases which had at least one "active" branch, but you're right this actually is confusing when there are no active branches.

> 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?

Right, I wasn't aware of that fix me. I was deliberately only excluding the termination directive from the skipped range and not the beginning, as I think it's nice to directly observe the fact that the condition evaluated to false/skipped.
But as mentioned above, i think it's confusing and in theory a "lie". Because preprocessor is definitely not skipping those directives, they're still processed (even in the cases where we have else/elif directives that come after an active branch).
So I suppose the right thing here is actually to preserve both start and termination directive for each block, to be consistent and also to give the correct semantics around "this PP-directive wasn't skipped".
WDYT?



================
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});
----------------
hokein wrote:
> 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`?
I wanted to perform the change here, because other consumers of skipped ranges should actually behave similar in clangd (not that we have more ATM).
We're definitely collecting information about PP-directives that start/terminate those skipped blocks, and special casing them in some places while not in others will probably result in bugs.

Do you see any reasons that we might actually want skipped ranges to include starting/terminating directives?


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:447
 $InactiveCode[[#if defined(test)]]
-$InactiveCode[[#endif]]
+#endif
     )cpp",
----------------
hokein wrote:
> 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.
> 
agreed see my main comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125863