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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D125863#3522384 <https://reviews.llvm.org/D125863#3522384>, @kadircet wrote:

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

Thanks. +1, yeah, I agree that this case is confusing.

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

Yeah, this sounds good to me.



================
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});
----------------
kadircet wrote:
> 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?
I don't have a strong rationale for this -- my feeling is that the CollectMacro class is the common data structure whose main responsibility is to collect information from the PPCallback, we'd better not to put specific-logic in that (otherwise skipped range concept is subtly different in CollectMacro/PPCallback, I think it might cause confusion).

Actually I think either can work as these skipped ranges are only used in semantic highlighting case. Maybe you're right -- we might want it for all cases.  So up to you (if we do it here, we need to update the document for MainFileMacros::SkippedRanges).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125863