ilya-biryukov added a comment.
Mostly comments about tests, the implementation itself LG.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:68
+llvm::Optional<SourceLocation> getSemiColonForDecl(const FunctionDecl *FD) {
+ const SourceManager &SM = FD->getASTContext().getSourceManager();
----------------
NIT: s/SemiColon/Semicolon
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:79
+ Token CurToken;
+ while (!CurToken.is(tok::semi)) {
+ if (RawLexer.LexFromRawLexer(CurToken))
----------------
What are the tokens we expect to see before the semicolon here?
It feels safer to just skip comments and whitespace and check the next token is
a semicolon.
Any reason to have an actual loop here?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:181
+ if (ND->getDeclContext() != Ref.Targets.front()->getDeclContext()) {
+ elog("Targets from multiple contexts: {0}, {1}",
+ printQualifiedName(*Ref.Targets.front()),
printQualifiedName(*ND));
----------------
NIT: could we mention the tweak name here? to make sure it's easy to detect
those lines in the logs:
`define inline: targets from multiple contexts`
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:208
+
+ if (HadErrors) {
+ return llvm::createStringError(
----------------
NIT: braces are redundant
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:320
Expected<Effect> apply(const Selection &Sel) override {
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "Not implemented yet");
+ const ASTContext &AST = Sel.AST.getASTContext();
+ const SourceManager &SM = AST.getSourceManager();
----------------
NIT: use auto
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:325
+ if (!SemiColon) {
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
----------------
NIT: braces are redundant
================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.cpp:85
+std::string TweakTest::apply(llvm::StringRef MarkedCode,
+ llvm::StringMap<std::string> *EditedFiles) const {
std::string WrappedCode = wrap(Context, MarkedCode);
----------------
Could you document `EditedFiles` contains all other edited files and the
original `std::string` only contains results for the main file?
Would also be useful to document what is the key in the StringMap:
absolute/relative paths or URI?
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:905
+ EXPECT_EQ(apply(R"cpp(
+ void foo() /*Comment -_-*/ ;
+
----------------
Why have a comment in this test?
If we need to test we handle comments before the semicolon, could we move to a
separate focused test?
If we want to test something else, let's leave out the comment. The test is
hard enough to read on its own
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:922
+
+ template <typename T> class Bar {};
+ Bar<int> z;
----------------
Template declarations inside function bodies are not allowed in C++.
Are we getting compiler errors here and not actually testing anything?
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:961
+
+ void foo() /*Comment -_-*/ ;
+
----------------
Same here: we don't need the comment here.
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:964
+ void f^oo() {
+ using namespace a;
+ bar<Bar<int>>.bar();
----------------
Either remove `using namespace a` from the function body or put a FIXME this
shouldn't actually qualify?
I believe the first option is what we want, if we're trying to check template
arguments are getting transformed properly
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:989
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+ return arg.first() == FileName && arg.second == Contents;
----------------
Maybe move this matcher to the start of the file?
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1029
+ EXPECT_THAT(EditedFiles,
+ testing::ElementsAre(FileWithContents(testPath("a.h"),
+ R"cpp(
----------------
We prefer to have `using ::testing::ElementsAre` at the start of the file and
not qualify the usage everywhere in our tests.
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1076
+ template <>
+ void foo<int>(int p);
+
----------------
This is really uncommon, I'm not even sure we should test this.
The other case is much more common and interesting:
```
template <class T>
void foo(T p);
template <>
void foo<int>(int p) { /* do something */ }
```
Could we add a test that we don't suggest this action in that case?
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1167
+ }
+ using namespace a;
+ using namespace b;
----------------
Can we test with a single namespace?
Having 3 layers of nested namespace in every test makes reading them very
complicated.
Instead, we could validate with separate tests:
- that the qualification logic works with multiple nested namespaces,
- that various kinds of references are properly qualified with a single
namespace
I believe that would keep both tests more focused
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1215
+ namespace b {
+ class Foo{};
+ namespace c {
----------------
Could you indent here and in other tests? it's hard to follow the fact that
`Foo` is inside `b` without indentation.
Same for other tests.
Alternatively, put on one line if there's just a single declaration inside the
namespace
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
