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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:208
+
+  if (HadErrors) {
+    return llvm::createStringError(
----------------
kadircet wrote:
> 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
It's more complicated than that... Let me explain...
I'm a fan of braces when:
1. There are line comments (I perceive those as an extra statement):
```
if (something) {
  // Here comes a comment...
  return 10;
}
```
2. There are composite statements inside the if body:
```
if (something) {
  for (;;)
    do_something_else();
}
```

In your case, I'd not use braces...
It's not important, though, pick the style you like most.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:922
+
+    template <typename T> class Bar {};
+    Bar<int> z;
----------------
kadircet wrote:
> 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.
I guess we didn't need to do any edits there, so we just carried the text unchanged.
Had we actually needed to change something inside the template, we'd fail to do so.

Range of the function body is correct, since the parser can easily recover in those cases.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1215
+    namespace b {
+    class Foo{};
+    namespace c {
----------------
kadircet wrote:
> 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
Yay! Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66647/new/

https://reviews.llvm.org/D66647