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