HighCommander4 wrote:
I spent a bit more time looking at this. I'd say I'm still quite far from
having a complete understanding of this code, but two things occur to me:
1. `TokenBuffer` operates at the level of the preprocessor.
"spelledForExpanded" is basically an operation which takes as input a range of
tokens in the preprocessed token stream ("expanded tokens"), and maps them back
to a range of tokens in the un-preprocessed token stream ("spelled tokens").
* The preprocessor has no notion of matching or balancing braces. Braces
only become relevant later, during parsing.
* So, I don't understand why an input token stream that contains unbalanced
braces would pose a problem for the "spelledForExpanded" operation.
* I'm hesitant to approve a patch that adds an early exit to
"spelledForExpanded" to prevent an out-of-bounds array access, without
understanding in more detail **how** the out-of-bounds array access arises /
how it's connected to the unbalanced braces. It's possible that the
out-of-bounds access is indicative of a deeper logic error which we'd be
covering up.
2. Since we've observed the problem arise during the `dumpAST` operation
(frame 10 of the stack trace
[here](https://github.com/clangd/clangd/issues/1559#issue-1646293852)), it
would be good to (also) add a test that exercises the `dumpAST` operation as a
whole.
* We have such tests in
[DumpASTTests.cpp](https://searchfox.org/llvm/source/clang-tools-extra/clangd/unittests/DumpASTTests.cpp).
* I think it would be preferable if the AST dump for an input with
unbalanced braces, such as `int main() {`, was **not** empty. (I suspect the
current patch may make it empty.) `clang -Xclang -ast-dump` manages to print
AST nodes in this case, I think so should we.
https://github.com/llvm/llvm-project/pull/69849
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
