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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
