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

Reply via email to