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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
