[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/ParsedAST.h:100
 
+  const std::vector<SourceRange> &getSkippedRanges() const {
+    return SkippedRanges;
----------------
hokein wrote:
> Instead of adding new member and methods in `ParsedAST`, I think we can do it in `CollectMainFileMacros` (adding a new field SkippRanges in `MainFileMacros`), then we can get the skipped ranges for preamble region within the main file for free.
This comment is undone.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:143
+    for (const SourceRange &R :
+         AST.getPreprocessor().getPreprocessingRecord()->getSkippedRanges()) {
+      if (isInsideMainFile(R.getBegin(), SM)) {
----------------
nridge wrote:
> hokein wrote:
> > I think the current implementation only collects the skipped preprocessing ranges **after preamble** region in the main file.  We probably need to store these ranges in PreambleData to make the ranges in the preamble region work, no need to do it in this patch, but we'd better have a test reflecting this fact. 
> > 
> > ```
> > #ifdef ActiveCOde
> > // inactive code here
> > #endif
> > 
> > #include "foo.h"
> > // preamble ends here
> > 
> > namespace ns {
> > // ..
> > }  
> > ```
> Your observation is correct: the current implementation only highlights inactive lines after the preamble. For now, I added a test case with a FIXME for this.
I'd prefer to fix it in this patch, it should not require large effort, and probably simplify the patch, you don't need to implement a new `PPCallbacks`.  See my another comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67536