Conversation
| if (typeIndex <= 0) | ||
| return ""; |
There was a problem hiding this comment.
This is an AI review. Take it with a grain of salt and feel free to reject it by resolving the comment.
The fix looks correct. I verified that the new crash2 test fails on main (built with -D_GLIBCXX_ASSERTIONS: Assertion '__n < this->size()' failed in getSpelling()) and passes with this PR.
The description says the guard is hoisted, but the two existing if (typeIndex <= 0) return ""; checks in the FunctionDecl/CXXConstructorDecl/CXXMethodDecl and DeclRefExpr branches above are kept. They are now redundant, since this new check covers them. Maybe remove them so there is only one guard? I tried that locally and TestClangImport still passes.
There was a problem hiding this comment.
Good point, those two are redundant now. Dropped both so there's just the single guard before the index. TestClangImport still passes.
There was a problem hiding this comment.
This is an AI review take it with a grain of salt. Please feel free to reject it by clicking on Resolve button
Thanks, the update looks good to me.
FYI: #8903 also adds a test right after crash() and a TEST_CASE line in the same place in test/testclangimport.cpp, so whichever of the two PRs is merged second will get a trivial merge conflict.
2 participants
getSpelling() sets typeIndex to mExtTokens.size() - 1 and, for node types other than the FunctionDecl and DeclRefExpr branches, reads mExtTokens[typeIndex - 1] without the typeIndex <= 0 check those two branches already apply. A clang AST dump whose node line carries no ext tokens leaves typeIndex at 0 or -1 (size() - 1 wraps into the int), so the read goes out of bounds, which ASAN flags as a SEGV while importing the dump via --clang. Hoist the existing guard ahead of the index so it covers every node type.