diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/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 &SemaRef, SourceLocation Begin, + SourceLocation End) { + auto &SrcMgr = 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 &SemaRef, 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 == IdealInitKeys[IdealIndex]) + break; + } + 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 + // valid or not. Just ignore it and reset IdealIndex to the previous + // valid initializer. + MissingInIdeal[InitIndex] = true; + IdealIndex = + InitsWithIdealIndex.empty() ? 0 : InitsWithIdealIndex.back().first; + continue; + } + } + + // If we found this initializer in an idealized list, but it was before the + // previous (valid) initializer, emit a warning. + if (IsOutOfOrder && !InitsWithIdealIndex.empty()) { + // Emit previous diagnostic, if any. + if (PDiag.second.getDiagID()) + SemaRef.Diag(PDiag.first, PDiag.second); + + const auto *PrevInit = InitsWithIdealIndex.back().second; + PDiag = {PrevInit->getSourceLocation(), + SemaRef.PDiag(diag::warn_initializer_out_of_order)}; if (PrevInit->isAnyMemberInitializer()) - D << 0 << PrevInit->getAnyMember()->getDeclName(); + PDiag.second << 0 << PrevInit->getAnyMember()->getDeclName(); else - D << 1 << PrevInit->getTypeSourceInfo()->getType(); + PDiag.second << 1 << PrevInit->getTypeSourceInfo()->getType(); if (Init->isAnyMemberInitializer()) - D << 0 << Init->getAnyMember()->getDeclName(); + PDiag.second << 0 << Init->getAnyMember()->getDeclName(); else - D << 1 << Init->getTypeSourceInfo()->getType(); + PDiag.second << 1 << Init->getTypeSourceInfo()->getType(); + } - // Move back to the initializer's location in the ideal list. - for (IdealIndex = 0; IdealIndex != NumIdealInits; ++IdealIndex) - if (InitKey == IdealInitKeys[IdealIndex]) - break; + InitsWithIdealIndex.push_back({IdealIndex, Init}); + } - assert(IdealIndex < NumIdealInits && - "initializer not found in initializer list"); + if (!PDiag.second.getDiagID()) + return; + + // If possible, add fix to the last diagnostic. The fix will reorder all + // initializers. This is preferable to attaching overlapping fixes for each + // initializer, which the user would then have to apply one by one. + auto &SrcMgr = SemaRef.getSourceManager(); + // Things get complicated when comments, macros or multiple files are + // involved. Check if that's the case and if so, do not emit the fix it. + // Ideally we would reorder the comments as well, but that is a lot more + // complicated. + // FIXME: Support re-ordering initializers with comments. + bool ShouldEmitFix = true; + for (const auto &Init : Inits) { + if (Init->getSourceLocation().isMacroID() || + !SrcMgr.isWrittenInSameFile(Init->getSourceLocation(), + Inits.front()->getSourceLocation())) { + ShouldEmitFix = false; + break; } + } + if (ShouldEmitFix) + ShouldEmitFix = + !RangeContainsComments(SemaRef, Inits.front()->getSourceLocation(), + Inits.back()->getSourceLocation()); - PrevInit = Init; + if (ShouldEmitFix) { + // Construct a FixIt for re-ordering the initializers. + llvm::sort(InitsWithIdealIndex.begin(), InitsWithIdealIndex.end()); + unsigned ReorderInitIndex = 0; + for (const auto &InitIndexPair : InitsWithIdealIndex) { + auto *InitToInsert = InitIndexPair.second; + const CXXCtorInitializer *InitToReplace = Inits[ReorderInitIndex]; + PDiag.second << FixItHint::CreateReplacement( + InitToReplace->getSourceRange(), + clang::Lexer::getSourceText( + CharSourceRange(InitToInsert->getSourceRange(), true), SrcMgr, + SemaRef.getLangOpts())); + + // Advance ReorderInitIndex to next valid initializer. + do { + ++ReorderInitIndex; + } while (ReorderInitIndex < Inits.size() && + MissingInIdeal[ReorderInitIndex]); + } } + + SemaRef.Diag(PDiag.first, PDiag.second); } namespace { diff --git a/clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp b/clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp --- a/clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp +++ b/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