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

Reply via email to