[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