[PATCH] D151190: [clangd] Do not end inactiveRegions range at position 0 of line

[Nathan Ridge via Phabricator via cfe-commits]

nridge planned changes to this revision.
nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1343
 #undef CMDMACRO
 $inactive3[[#ifdef CMDMACRO
   int inactiveInt2;
----------------
daiyousei-qz wrote:
> hokein wrote:
> > While this patch is an improvement, I wonder we should move it further.
> > 
> > Has been thinking about it more, we seem to have some inconsistent behavior on highlighting the `#if`, `#else`, `#endif` lines:
> > 
> > - in `$inactive1` case, the `#endif` is marked as inactive (IMO, the highlighting in the editor is confusing, it looks like we're missing a match `endif`, thus I prefer to mark it as active to correspond to the active `#if` branch);
> > - in `$inactive3` case, the line `#elif PREAMBLEMACRO > 0` is marked as inactive, this is inconsistent with `$inactive1` case;
> > 
> > I think it would be nice to have a consistent model. One approach is to always consider `#if`, `#elif`, `#endif`, `#endif` lines active (in the implementation side, this would mean we always use the line range [skipp_range.start.line+1, skipp_range.end.line - 1]).
> > 
> > What do you think about this?
> > 
> > 
> +1. My perspective is that C++ source code is actually a meta-language (preprocessor) that describes generation of C++ language code. That is, `#if`, `#else`, `#endif` and .etc are always in a sense "executed" to generate the actual code. So they shouldn't be marked as inactive.
I agree, what you describe is a nice additional improvement.

I believe it would also resolve the long-standing issue https://github.com/clangd/clangd/issues/773.

I will update the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151190