kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:79
+ Token CurToken;
+ while (!CurToken.is(tok::semi)) {
+ if (RawLexer.LexFromRawLexer(CurToken))
----------------
ilya-biryukov wrote:
> 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?
it has been a long time since our offline discussions around this one. but the
idea was could have any attributes before semicolon, these could be macros that
will expand to one thing in some platforms and another in a different platform.
Therefore we've decided to skip anything until we've found a semicolon.
I don't think I follow the second part of the question though? What do you mean
by "having an actual loop"?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:208
+
+ if (HadErrors) {
+ return llvm::createStringError(
----------------
ilya-biryukov wrote:
> NIT: braces are redundant
I've thought you were a fan of braces when the inner statement spans multiple
lines, even if it is a single statement :D
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:922
+
+ template <typename T> class Bar {};
+ Bar<int> z;
----------------
ilya-biryukov wrote:
> Template declarations inside function bodies are not allowed in C++.
> Are we getting compiler errors here and not actually testing anything?
well, it was still working and inlining the function definition, apparently
despite the diagnostic, ranges were correct.
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1076
+ template <>
+ void foo<int>(int p);
+
----------------
ilya-biryukov wrote:
> 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?
yes there is a test for this in availability checks patch:
```
EXPECT_UNAVAILABLE(R"cpp(
template <typename T> void foo();
template<> void f^oo<int>() {
})cpp");
```
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1215
+ namespace b {
+ class Foo{};
+ namespace c {
----------------
ilya-biryukov wrote:
> 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
no more nested namespaces
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
