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