[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Kadir Cetinkaya via Phabricator via cfe-commits]

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