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