ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
A few last NITs and one important comment about handling the case when function
definition come from macro expansions
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:323
+
+ auto SemiColon = getSemicolonForDecl(Target);
+ if (!SemiColon) {
----------------
NIT: s/SemiColon/Semicolon
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:334
+
+ const tooling::Replacement SemiColonToFuncBody(SM, *SemiColon, 1,
+ *QualifiedBody);
----------------
Could we add a test when the semicolon comes from a macro expansion? I believe
the action won't be available in that case, which is ok. Just to make sure we
cover this corner-case.
```
#define SEMI ;
void foo() SEMI
```
Also interesting cases:
- source function is in a macro expansion
- target function is in a macro expansion.
I believe they might catch a few bugs here, as we seem to assume all locations
are file locations...
================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:73
+ // for the main file.
+ // Populates \p EditedFiles if there were changes to other files whenever
+ // it is non-null. It is a mapping from absolute path of the edited file
to
----------------
Hm, maybe even call `ADD_FAILURE()` when there are changes to other files and
tests pass `null` to `EditedFiles`?
Likely an error in the test.
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1226
+ void foo(){
+ return;
+ }
----------------
Argh... Without `clang-format` that looks SO weird :-)
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1266
+ template <typename T>
+ void foo();
+
----------------
Maybe put it before `using namespace a;`?
To make sure the test does not need to change if we actually support not
qualifying when not neccessary
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1332
+ template <>
+ void fo^o<char>(char p) {
+ return;
----------------
How is this test different from the previous one?
Is it just trying to make sure we don't always move to the first function body?
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1374
+ )cpp");
+}
+
----------------
Do we test the following case anywhere?
```
template <class T> void foo();
template <> void ^foo<int>() { // should not be available
return;
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66647/new/
https://reviews.llvm.org/D66647
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
