Page MenuHomePhabricator

[clangd] Dont mark terminating PP-directives as skipped
Needs ReviewPublic

Authored by kadircet on May 18 2022, 2:21 AM.

Details

Reviewers
sammccall
hokein
Summary

PP-callback includes the terminating PP-directive
(else/elifdef/elifndef/endif) in the skipped source range. This results in
confusion as in theory that PP-directive is not skipped.
This patch changes the end location to be start of the line containing the
termination directive. That way clangd can keep highlightings for macro names in
the terminating directive as well.
This patch doesn't change the semantics of the PP-callback, as the range
possibly contains comments etc. trailing the termination directive. It's unclear
how useful that's for applications (only coverage mapping makes use of the full
range, rest always uses the endifloc for termination location), but it
definitely looks like a more intrusive change than just handling in clangd.

Diff Detail

Event Timeline

kadircet created this revision.May 18 2022, 2:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 2:21 AM
kadircet requested review of this revision.May 18 2022, 2:21 AM

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), I think we're trying to fix that?

clang-tools-extra/clangd/CollectMacros.h
90

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?

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
447

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.

kadircet marked an inline comment as done.May 18 2022, 7:35 AM

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

clang-tools-extra/clangd/CollectMacros.h
90

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?

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
447

agreed see my main 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.

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

clang-tools-extra/clangd/CollectMacros.h
90

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

daiyousei-qz added a subscriber: daiyousei-qz.EditedJun 5 2022, 11:53 PM

I have a similar change recently presented in https://github.com/clangd/clangd/issues/1181 that makes all conditional preprocessor lines active. Please refer to the change if that's helpful :)