[PATCH] D87244: [clang] Add fix-it for -Wreorder-ctor.
Jeroen added inline comments.
Comment at: clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp:171
+ // code can't do this.
+ Bar(int x) : c(x), /*foo*/ a(2), b(3) {} // expected-warning {{field 'c'
will be initialized after field 'a'}}
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
What would be the behavior for `:c(x), b(3), a(2)`?
Does that get fixed in 1 go, or should this be done in 2 steps?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87244/new/
https://reviews.llvm.org/D87244
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87244: [clang] Add fix-it for -Wreorder-ctor.
kadircet added a comment.
thanks, comments around some implementations. the only high level question i
have is about the choice of location for fix-it (see the detailed comment
inline)
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5198
+static bool RangeContainsComments(Sema , SourceLocation Begin,
+ SourceLocation End) {
+ auto = SemaRef.getSourceManager();
nit: assert for fileids of begin and end at the beginning.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5208
+ while (!Lex.LexFromRawLexer(Tok) &&
+ SrcMgr.isBeforeInTranslationUnit(Tok.getLocation(), End)) {
+if (Tok.is(tok::comment))
since Tok and End are in the same file you can just use `<` operator.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5298
+ if (IdealIndex == NumIdealInits) {
+// This initializer is not in the ideal list at all. This can happen in
+// a class with dependent base, when we cannot tell if initializer is
i don't really follow when this happens. comments mentions a dependent base,
but test case only has a templated constructor. why do we think it is safe to
continue in this case, while we bail out in case of a dependent class? i would
suggest just bailing out at the beginning of the function if constructor is
dependent too (in addition to it's class being dependent)
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5311
+// previous (valid) initializer, emit a warning.
+if (IsOutOfOrder && !InitsWithIdealIndex.empty()) {
+ // Emit previous diagnostic, if any.
why not move this into previous if block i.e.
```
if (isoutoforder) {
if(!Initswithidealindex.empty()) { ... }
if(IdealIndex== NumIdealInits) { ..}
if(!InitsWithIdealIndex.empty() {
// emit diag
}
}
```
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5337
+
+ // If possible, add fix to the last diagnostic. The fix will reorder all
+ // initializers. This is preferable to attaching overlapping fixes for each
I totally agree with having a single fix-it to re-order all of the
initializers, but I am not sure if last initializer is the best location for
that fixit, especially for interactive tools (it is definitely the right
behaviour for command line tools like clang and clang-tidy tho, as they'll
either print the fix multiple times or try to apply conflicting fixes)
e.g. if user sees 3 out-of-order initializer warnings, they'll likely hover
over one of them and say, "aaah" and fix the code themselves, if fixit wasn't
available with that one.
Moreover when there's only 1 out-of-order initialzier warning, they'll see the
fixit attached no matter what.
Next time, when there are multiple warnings they won't see it and will likely
file bugs saying that "clang(d) doesn't generate fixes all the time".
But I also don't have any better suggestions, as having it multiple times will
definitely be helpful for interactive tools but will regress the others.. Maybe
attaching it to the first initializer might be better overall?
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5348
+ for (const auto : Inits) {
+if (Init->getSourceLocation().isMacroID() ||
+!SrcMgr.isWrittenInSameFile(Init->getSourceLocation(),
can we have some tests for this case?
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5355
+ }
+ if (ShouldEmitFix)
+ShouldEmitFix =
nit: `ShouldEmitFix = ShouldEmitFix && !Range...`
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5358
+!RangeContainsComments(SemaRef, Inits.front()->getSourceLocation(),
+ Inits.back()->getSourceLocation());
what if there are comments after last initializer ? I think we should rather
use LBraceLoc for the constructor in here.
Comment at: clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp:139
+struct Y {
+ template Y(T x) : a(), X(x) {}
+ Foo a;
i think it is enough to have `int a;` as a member do we really need a separate
type? + why don't we just merge this with the previous case?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87244/new/
https://reviews.llvm.org/D87244
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87244: [clang] Add fix-it for -Wreorder-ctor.
adamcz updated this revision to Diff 290313.
adamcz added a comment.
Add a missing const
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87244/new/
https://reviews.llvm.org/D87244
Files:
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
Index: clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
===
--- clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
+++ clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -fsyntax-only -Wreorder -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -Wreorder %s 2>&1 | FileCheck %s
struct BB {};
@@ -122,6 +123,9 @@
}
namespace PR7179 {
+struct Foo {
+ Foo();
+};
struct X
{
struct Y
@@ -129,6 +133,13 @@
template Y(T x) : X(x) { }
};
};
+
+ struct X2 {
+struct Y {
+ template Y(T x) : a(), X(x) {}
+ Foo a;
+};
+ };
}
namespace test3 {
@@ -141,3 +152,23 @@
}
};
}
+
+namespace fix {
+struct Foo {
+ Foo(int) {}
+};
+struct Bar {
+ Foo a, b, c;
+
+ Bar() : c(1), a(2), b(3) {} // expected-warning {{field 'c' will be initialized after field 'a'}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:17}:"a(2)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:23}:"b(3)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:25-[[@LINE-3]]:29}:"c(1)"
+
+ // Check that we do not generate fix it when comments are present. Ideally
+ // we would, but comments would need to be reordered as well and the current
+ // code can't do this.
+ Bar(int x) : c(x), /*foo*/ a(2), b(3) {} // expected-warning {{field 'c' will be initialized after field 'a'}}
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+};
+} // namespace fix
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -5192,6 +5192,26 @@
return Member->getAnyMember()->getCanonicalDecl();
}
+// Returns true iff there are comments between Begin and End. Expects that both
+// Begin and End are in the same file.
+static bool RangeContainsComments(Sema , SourceLocation Begin,
+ SourceLocation End) {
+ auto = SemaRef.getSourceManager();
+ auto BeginLocInfo = SrcMgr.getDecomposedLoc(Begin);
+ StringRef File = SrcMgr.getBufferData(BeginLocInfo.first);
+ Lexer Lex(SrcMgr.getLocForStartOfFile(BeginLocInfo.first),
+SemaRef.getLangOpts(), File.begin(),
+File.begin() + BeginLocInfo.second, File.end());
+ Lex.SetCommentRetentionState(true);
+ Token Tok;
+ while (!Lex.LexFromRawLexer(Tok) &&
+ SrcMgr.isBeforeInTranslationUnit(Tok.getLocation(), End)) {
+if (Tok.is(tok::comment))
+ return true;
+ }
+ return false;
+}
+
static void DiagnoseBaseOrMemInitializerOrder(
Sema , const CXXConstructorDecl *Constructor,
ArrayRef Inits) {
@@ -5241,7 +5261,20 @@
unsigned NumIdealInits = IdealInitKeys.size();
unsigned IdealIndex = 0;
- CXXCtorInitializer *PrevInit = nullptr;
+ // Instead of emitting diagnostic right away, we keep it here and only emit
+ // when we find a new one. The last one is emitted outside of this loop, with
+ // a FixIt attached.
+ PartialDiagnosticAt PDiag = {SourceLocation(),
+ PartialDiagnostic::NullDiagnostic()};
+ // Pairs {IdealIndex, pointer-to-initializer}, valid (i.e. found in
+ // IdealInitKeys) initializers only. Last value is used to generate
+ // diagnostics, the index is then used to sort valid initializers in
+ // initialization order.
+ SmallVector, 32>
+ InitsWithIdealIndex;
+ // MissingInIdeal[I] == true iff Inits[I] is not found in ideal list.
+ llvm::SmallBitVector MissingInIdeal(Inits.size());
+
for (unsigned InitIndex = 0; InitIndex != Inits.size(); ++InitIndex) {
CXXCtorInitializer *Init = Inits[InitIndex];
const void *InitKey = GetKeyForMember(SemaRef.Context, Init);
@@ -5252,35 +5285,100 @@
if (InitKey == IdealInitKeys[IdealIndex])
break;
-// If we didn't find this initializer, it must be because we
-// scanned past it on a previous iteration. That can only
-// happen if we're out of order; emit a warning.
-if (IdealIndex == NumIdealInits && PrevInit) {
- Sema::SemaDiagnosticBuilder D =
-SemaRef.Diag(PrevInit->getSourceLocation(),
- diag::warn_initializer_out_of_order);
+const bool IsOutOfOrder = IdealIndex == NumIdealInits;
+if (IsOutOfOrder) {
+ if (!InitsWithIdealIndex.empty()) {
+// This is not the first initializer we process. It's either not in the
+// ideal list at all, or it's out of order. Scan from the beginning.
+for (IdealIndex = 0; IdealIndex != NumIdealInits; ++IdealIndex)
+ if (InitKey ==
[PATCH] D87244: [clang] Add fix-it for -Wreorder-ctor.
adamcz updated this revision to Diff 290312.
adamcz added a comment.
fixed comment typo
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87244/new/
https://reviews.llvm.org/D87244
Files:
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
Index: clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
===
--- clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
+++ clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -fsyntax-only -Wreorder -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -Wreorder %s 2>&1 | FileCheck %s
struct BB {};
@@ -122,6 +123,9 @@
}
namespace PR7179 {
+struct Foo {
+ Foo();
+};
struct X
{
struct Y
@@ -129,6 +133,13 @@
template Y(T x) : X(x) { }
};
};
+
+ struct X2 {
+struct Y {
+ template Y(T x) : a(), X(x) {}
+ Foo a;
+};
+ };
}
namespace test3 {
@@ -141,3 +152,23 @@
}
};
}
+
+namespace fix {
+struct Foo {
+ Foo(int) {}
+};
+struct Bar {
+ Foo a, b, c;
+
+ Bar() : c(1), a(2), b(3) {} // expected-warning {{field 'c' will be initialized after field 'a'}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:17}:"a(2)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:23}:"b(3)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:25-[[@LINE-3]]:29}:"c(1)"
+
+ // Check that we do not generate fix it when comments are present. Ideally
+ // we would, but comments would need to be reordered as well and the current
+ // code can't do this.
+ Bar(int x) : c(x), /*foo*/ a(2), b(3) {} // expected-warning {{field 'c' will be initialized after field 'a'}}
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+};
+} // namespace fix
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -5192,6 +5192,26 @@
return Member->getAnyMember()->getCanonicalDecl();
}
+// Returns true iff there are comments between Begin and End. Expects that both
+// Begin and End are in the same file.
+static bool RangeContainsComments(Sema , SourceLocation Begin,
+ SourceLocation End) {
+ auto = SemaRef.getSourceManager();
+ auto BeginLocInfo = SrcMgr.getDecomposedLoc(Begin);
+ StringRef File = SrcMgr.getBufferData(BeginLocInfo.first);
+ Lexer Lex(SrcMgr.getLocForStartOfFile(BeginLocInfo.first),
+SemaRef.getLangOpts(), File.begin(),
+File.begin() + BeginLocInfo.second, File.end());
+ Lex.SetCommentRetentionState(true);
+ Token Tok;
+ while (!Lex.LexFromRawLexer(Tok) &&
+ SrcMgr.isBeforeInTranslationUnit(Tok.getLocation(), End)) {
+if (Tok.is(tok::comment))
+ return true;
+ }
+ return false;
+}
+
static void DiagnoseBaseOrMemInitializerOrder(
Sema , const CXXConstructorDecl *Constructor,
ArrayRef Inits) {
@@ -5241,7 +5261,20 @@
unsigned NumIdealInits = IdealInitKeys.size();
unsigned IdealIndex = 0;
- CXXCtorInitializer *PrevInit = nullptr;
+ // Instead of emitting diagnostic right away, we keep it here and only emit
+ // when we find a new one. The last one is emitted outside of this loop, with
+ // a FixIt attached.
+ PartialDiagnosticAt PDiag = {SourceLocation(),
+ PartialDiagnostic::NullDiagnostic()};
+ // Pairs {IdealIndex, pointer-to-initializer}, valid (i.e. found in
+ // IdealInitKeys) initializers only. Last value is used to generate
+ // diagnostics, the index is then used to sort valid initializers in
+ // initialization order.
+ SmallVector, 32>
+ InitsWithIdealIndex;
+ // MissingInIdeal[I] == true iff Inits[I] is not found in ideal list.
+ llvm::SmallBitVector MissingInIdeal(Inits.size());
+
for (unsigned InitIndex = 0; InitIndex != Inits.size(); ++InitIndex) {
CXXCtorInitializer *Init = Inits[InitIndex];
const void *InitKey = GetKeyForMember(SemaRef.Context, Init);
@@ -5252,35 +5285,100 @@
if (InitKey == IdealInitKeys[IdealIndex])
break;
-// If we didn't find this initializer, it must be because we
-// scanned past it on a previous iteration. That can only
-// happen if we're out of order; emit a warning.
-if (IdealIndex == NumIdealInits && PrevInit) {
- Sema::SemaDiagnosticBuilder D =
-SemaRef.Diag(PrevInit->getSourceLocation(),
- diag::warn_initializer_out_of_order);
+bool IsOutOfOrder = IdealIndex == NumIdealInits;
+if (IsOutOfOrder) {
+ if (!InitsWithIdealIndex.empty()) {
+// This is not the first initializer we process. It's either not in the
+// ideal list at all, or it's out of order. Scan from the beginning.
+for (IdealIndex = 0; IdealIndex != NumIdealInits; ++IdealIndex)
+ if (InitKey ==
[PATCH] D87244: [clang] Add fix-it for -Wreorder-ctor.
adamcz created this revision.
Herald added subscribers: cfe-commits, mgrang.
Herald added a project: clang.
adamcz requested review of this revision.
This version is very limited. It does not work when comments are present
anywhere in the initializer list, since we do not have a good way to
associate them to specific initalizer and move them around.
The fix-it is associated with the last ctor-order diagnostic and
reorders all initializers. This is better than providing fix for each
diagnostic generated, since user would have to apply them one-by-one.
This also fixes a bug that would trigger an assert() in dependent
classes, when initializer present in code might not be found in the
idealized list of initializers. That is related to PR7179.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D87244
Files:
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
Index: clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
===
--- clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
+++ clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -fsyntax-only -Wreorder -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -Wreorder %s 2>&1 | FileCheck %s
struct BB {};
@@ -122,6 +123,9 @@
}
namespace PR7179 {
+struct Foo {
+ Foo();
+};
struct X
{
struct Y
@@ -129,6 +133,13 @@
template Y(T x) : X(x) { }
};
};
+
+ struct X2 {
+struct Y {
+ template Y(T x) : a(), X(x) {}
+ Foo a;
+};
+ };
}
namespace test3 {
@@ -141,3 +152,23 @@
}
};
}
+
+namespace fix {
+struct Foo {
+ Foo(int) {}
+};
+struct Bar {
+ Foo a, b, c;
+
+ Bar() : c(1), a(2), b(3) {} // expected-warning {{field 'c' will be initialized after field 'a'}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:17}:"a(2)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:23}:"b(3)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:25-[[@LINE-3]]:29}:"c(1)"
+
+ // Check that we do not generate fix it when comments are present. Ideally
+ // we would, but comments would need to be reordered as well and the current
+ // code can't do this.
+ Bar(int x) : c(x), /*foo*/ a(2), b(3) {} // expected-warning {{field 'c' will be initialized after field 'a'}}
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+};
+} // namespace fix
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -5192,6 +5192,26 @@
return Member->getAnyMember()->getCanonicalDecl();
}
+// Returns true iff there are comments between Begin and End. Expects that both
+// Begin and End are in the same file.
+static bool RangeContainsComments(Sema , SourceLocation Begin,
+ SourceLocation End) {
+ auto = SemaRef.getSourceManager();
+ auto BeginLocInfo = SrcMgr.getDecomposedLoc(Begin);
+ StringRef File = SrcMgr.getBufferData(BeginLocInfo.first);
+ Lexer Lex(SrcMgr.getLocForStartOfFile(BeginLocInfo.first),
+SemaRef.getLangOpts(), File.begin(),
+File.begin() + BeginLocInfo.second, File.end());
+ Lex.SetCommentRetentionState(true);
+ Token Tok;
+ while (!Lex.LexFromRawLexer(Tok) &&
+ SrcMgr.isBeforeInTranslationUnit(Tok.getLocation(), End)) {
+if (Tok.is(tok::comment))
+ return true;
+ }
+ return false;
+}
+
static void DiagnoseBaseOrMemInitializerOrder(
Sema , const CXXConstructorDecl *Constructor,
ArrayRef Inits) {
@@ -5241,7 +5261,20 @@
unsigned NumIdealInits = IdealInitKeys.size();
unsigned IdealIndex = 0;
- CXXCtorInitializer *PrevInit = nullptr;
+ // Instead of emitting diagnostic right away, we keep it here and only emit
+ // when we find a new one. The last one is emitted outsdie of this loop, with
+ // a FixIt attached.
+ PartialDiagnosticAt PDiag = {SourceLocation(),
+ PartialDiagnostic::NullDiagnostic()};
+ // Pairs {IdealIndex, pointer-to-initializer}, valid (i.e. found in
+ // IdealInitKeys) initializers only. Last value is used to generate
+ // diagnostics, the index is then used to sort valid initializers in
+ // initialization order.
+ SmallVector, 32>
+ InitsWithIdealIndex;
+ // MissingInIdeal[I] == true iff Inits[I] is not found in ideal list.
+ llvm::SmallBitVector MissingInIdeal(Inits.size());
+
for (unsigned InitIndex = 0; InitIndex != Inits.size(); ++InitIndex) {
CXXCtorInitializer *Init = Inits[InitIndex];
const void *InitKey = GetKeyForMember(SemaRef.Context, Init);
@@ -5252,35 +5285,100 @@
if (InitKey == IdealInitKeys[IdealIndex])
break;
-// If we didn't find this initializer, it must be because we
-// scanned past it on a previous iteration. That can only
-// happen if we're out of order; emit a warning.
-if
