Fix code that attempted to produce a diagnostic with one DiagnosticEngine, then

produce a note for that diagnostic either with a different DiagnosticEngine or
after calling DiagnosticEngine::Reset(). That didn't make any sense, and did the
wrong thing if the original diagnostic was suppressed.

llvm-svn: 170636

clang/include/clang/AST/ASTImporter.h

@@ -48,6 +48,9 @@ namespace clang {
 
     /// \brief Whether to perform a minimal import.
     bool Minimal;
+
+    /// \brief Whether the last diagnostic came from the "from" context.
+    bool LastDiagFromFrom;
     
     /// \brief Mapping from the already-imported types in the "from" context
     /// to the corresponding types in the "to" context.

clang/include/clang/Basic/Diagnostic.h

@@ -607,6 +607,12 @@ public:
     ArgToStringCookie = Cookie;
   }
 
+  /// \brief Note that the prior diagnostic was emitted by some other
+  /// \c DiagnosticsEngine, and we may be attaching a note to that diagnostic.
+  void notePriorDiagnosticFrom(const DiagnosticsEngine &Other) {
+    LastDiagLevel = Other.LastDiagLevel;
+  }
+
   /// \brief Reset the state of the diagnostic object to its initial 
   /// configuration.
   void Reset();

clang/include/clang/Basic/DiagnosticASTKinds.td

@@ -135,7 +135,8 @@ def err_odr_function_type_inconsistent : Error<
   "external function %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
 def warn_odr_tag_type_inconsistent : Warning<
-  "type %0 has incompatible definitions in different translation units">;
+  "type %0 has incompatible definitions in different translation units">,
+  InGroup<DiagGroup<"odr">>;
 def note_odr_tag_kind_here: Note<
   "%0 is a %select{struct|interface|union|class|enum}1 here">;
 def note_odr_field : Note<"field %0 has type %1 here">;

clang/lib/AST/ASTImporter.cpp

@@ -206,12 +206,16 @@ namespace {
     /// \brief Whether to complain about failures.
     bool Complain;
 
+    /// \brief \c true if the last diagnostic came from C2.
+    bool LastDiagFromC2;
+
     StructuralEquivalenceContext(ASTContext &C1, ASTContext &C2,
                llvm::DenseSet<std::pair<Decl *, Decl *> > &NonEquivalentDecls,
                                  bool StrictTypeSpelling = false,
                                  bool Complain = true)
       : C1(C1), C2(C2), NonEquivalentDecls(NonEquivalentDecls),
-        StrictTypeSpelling(StrictTypeSpelling), Complain(Complain) { }
+        StrictTypeSpelling(StrictTypeSpelling), Complain(Complain),
+        LastDiagFromC2(false) {}
 
     /// \brief Determine whether the two declarations are structurally
     /// equivalent.
@@ -229,11 +233,17 @@ namespace {
   public:
     DiagnosticBuilder Diag1(SourceLocation Loc, unsigned DiagID) {
       assert(Complain && "Not allowed to complain");
+      if (LastDiagFromC2)
+        C1.getDiagnostics().notePriorDiagnosticFrom(C2.getDiagnostics());
+      LastDiagFromC2 = false;
       return C1.getDiagnostics().Report(Loc, DiagID);
     }
 
     DiagnosticBuilder Diag2(SourceLocation Loc, unsigned DiagID) {
       assert(Complain && "Not allowed to complain");
+      if (!LastDiagFromC2)
+        C2.getDiagnostics().notePriorDiagnosticFrom(C1.getDiagnostics());
+      LastDiagFromC2 = true;
       return C2.getDiagnostics().Report(Loc, DiagID);
     }
   };
@@ -4319,7 +4329,7 @@ ASTImporter::ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
                          bool MinimalImport)
   : ToContext(ToContext), FromContext(FromContext),
     ToFileManager(ToFileManager), FromFileManager(FromFileManager),
-    Minimal(MinimalImport) 
+    Minimal(MinimalImport), LastDiagFromFrom(false)
 {
   ImportedDecls[FromContext.getTranslationUnitDecl()]
     = ToContext.getTranslationUnitDecl();
@@ -4822,10 +4832,18 @@ DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
 }
 
 DiagnosticBuilder ASTImporter::ToDiag(SourceLocation Loc, unsigned DiagID) {
+  if (LastDiagFromFrom)
+    ToContext.getDiagnostics().notePriorDiagnosticFrom(
+      FromContext.getDiagnostics());
+  LastDiagFromFrom = false;
   return ToContext.getDiagnostics().Report(Loc, DiagID);
 }
 
 DiagnosticBuilder ASTImporter::FromDiag(SourceLocation Loc, unsigned DiagID) {
+  if (!LastDiagFromFrom)
+    FromContext.getDiagnostics().notePriorDiagnosticFrom(
+      ToContext.getDiagnostics());
+  LastDiagFromFrom = true;
   return FromContext.getDiagnostics().Report(Loc, DiagID);
 }
 

clang/lib/Basic/Diagnostic.cpp

@@ -108,11 +108,7 @@ void DiagnosticsEngine::Reset() {
   TrapNumUnrecoverableErrorsOccurred = 0;
   
   CurDiagID = ~0U;
-  // Set LastDiagLevel to an "unset" state. If we set it to 'Ignored', notes
-  // using a DiagnosticsEngine associated to a translation unit that follow
-  // diagnostics from a DiagnosticsEngine associated to anoter t.u. will not be
-  // displayed.
-  LastDiagLevel = (DiagnosticIDs::Level)-1;
+  LastDiagLevel = DiagnosticIDs::Ignored;
   DelayedDiagID = 0;
 
   // Clear state related to #pragma diagnostic.

clang/lib/Driver/Driver.cpp

@@ -366,9 +366,12 @@ void Driver::generateCompilationDiagnostics(Compilation &C,
     C.PrintDiagnosticJob(OS, C.getJobs());
   OS.flush();
 
-  // Clear stale state and suppress tool output.
+  // Keep track of whether we produce any errors while trying to produce
+  // preprocessed sources.
+  DiagnosticErrorTrap Trap(Diags);
+
+  // Suppress tool output.
   C.initCompilationForDiagnostics();
-  Diags.Reset();
 
   // Construct the list of inputs.
   InputList Inputs;
@@ -430,7 +433,7 @@ void Driver::generateCompilationDiagnostics(Compilation &C,
   BuildJobs(C);
 
   // If there were errors building the compilation, quit now.
-  if (Diags.hasErrorOccurred()) {
+  if (Trap.hasErrorOccurred()) {
     Diag(clang::diag::note_drv_command_failed_diag_msg)
       << "Error generating preprocessed source(s).";
     return;

clang/test/ASTMerge/Inputs/class1.cpp

@@ -13,3 +13,7 @@ struct C {
   C &operator=(C&);
   ~C();
 };
+
+enum E {
+  b = 1
+};

clang/test/ASTMerge/Inputs/class2.cpp

@@ -7,3 +7,7 @@ struct B : A {
   int foo();
 };
 
+enum E {
+  a = 0,
+  b = 1
+};

clang/test/ASTMerge/class.cpp

@@ -1,9 +1,14 @@
 // RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/class1.cpp
 // RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/class2.cpp
 // RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 -Wno-odr | count 0
 
 // CHECK: class1.cpp:5:8: warning: type 'B' has incompatible definitions in different translation units
 // CHECK: class1.cpp:6:9: note: field 'y' has type 'float' here
 // CHECK: class2.cpp:6:7: note: field 'y' has type 'int' here
 
 // FIXME: we should also complain about mismatched types on the method
+
+// CHECK: class1.cpp:17:6: warning: type 'E' has incompatible definitions in different translation units
+// CHECK: class1.cpp:18:3: note: enumerator 'b' with value 1 here
+// CHECK: class2.cpp:11:3: note: enumerator 'a' with value 0 here

clang/test/Misc/warning-flags.c

@@ -18,7 +18,7 @@ This test serves two purposes:
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (148):
+CHECK: Warnings without flags (147):
 CHECK-NEXT:   ext_delete_void_ptr_operand
 CHECK-NEXT:   ext_enum_friend
 CHECK-NEXT:   ext_expected_semi_decl_list
@@ -106,7 +106,6 @@ CHECK-NEXT:   warn_not_compound_assign
 CHECK-NEXT:   warn_objc_property_copy_missing_on_block
 CHECK-NEXT:   warn_objc_protocol_qualifier_missing_id
 CHECK-NEXT:   warn_octal_escape_too_large
-CHECK-NEXT:   warn_odr_tag_type_inconsistent
 CHECK-NEXT:   warn_on_superclass_use
 CHECK-NEXT:   warn_param_default_argument_redefinition
 CHECK-NEXT:   warn_partial_specs_not_deducible