[clang] [clang-tools-extra] [CLANGD] Do not crash on designator initialization of union (PR #83369)
Nathan Ridge via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 12 23:24:17 PDT 2024
https://github.com/HighCommander4 requested changes to this pull request.
Thanks for the patch!
I wrote some notes in the issue (https://github.com/llvm/llvm-project/issues/83185) to help me understand the problem the patch is fixing.
The fix looks good to me.
Regarding the test:
* Usually, for fixes in frontend code (`clang` directory), it's preferred to have a test which is also in `clang`. For an issue like this (an AST node having a bad source location), a common way to test it is a lit test with runs `ast-dump` on the source file and checks the node's source location in the AST dump.
* However, `DesignatedInitExpr` does not appear at all in the AST dump. (The reason is that it's only part of the syntactic form of `InitListExpr`, and the AST dumping code only prints the semantic form.)
* Therefore, I would say writing a clangd test is fine.
* For clangd tests, we generally prefer unit tests. Could you put the testcase in `ExtractVariableTests.cpp`, like [this](https://searchfox.org/llvm/rev/ffe41819e58365dfbe85a22556c0d9d284e746b9/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp#68-73)? The input is the source code with the selection that causes the crash annotated with `[[]]`, and then you can call `EXPECT_AVAILABLE()` on it (with the fix, the refactoring is in fact available for this selection).
Thanks!
https://github.com/llvm/llvm-project/pull/83369
More information about the cfe-commits
mailing list