Now that the structure of this is more reasonably laid out, fix a long

standing deficiency: we were providing no macro backtrace information
whenever caret diagnostics were turned off. This sinks the logic for
suppressing the code snippet and caret to the code that actually prints
tho code snippet and caret. Along the way, clean up the naming of
functions, remove some now fixed FIXMEs, and generally improve the
wording and logic of this process.

Add a test case exerecising this functionality. It is notable that the
resulting messages are extremely low quality. I'm working on a follow-up
patch that should address this and have left a FIXME in the test case.

llvm-svn: 142120

3 files changed, 55 insertions, 33 deletions

diff --git a/clang/include/clang/Frontend/TextDiagnostic.h b/clang/include/clang/Frontend/TextDiagnostic.h
index 84e23b1..ce86914 100644
--- a/clang/include/clang/Frontend/TextDiagnostic.h
+++ b/clang/include/clang/Frontend/TextDiagnostic.h
@@ -127,12 +127,13 @@ private:
   void emitDiagnosticLoc(SourceLocation Loc, PresumedLoc PLoc,
                          DiagnosticsEngine::Level Level,
                          ArrayRef<CharSourceRange> Ranges);
-  void emitCaret(SourceLocation Loc,
-                 SmallVectorImpl<CharSourceRange>& Ranges,
-                 ArrayRef<FixItHint> Hints,
-                 unsigned &MacroDepth,
-                 unsigned OnMacroInst = 0);
-  void emitSnippetAndCaret(SourceLocation Loc,
+  void emitMacroExpansionsAndCarets(SourceLocation Loc,
+                                    DiagnosticsEngine::Level Level,
+                                    SmallVectorImpl<CharSourceRange>& Ranges,
+                                    ArrayRef<FixItHint> Hints,
+                                    unsigned &MacroDepth,
+                                    unsigned OnMacroInst = 0);
+  void emitSnippetAndCaret(SourceLocation Loc, DiagnosticsEngine::Level Level,
                            SmallVectorImpl<CharSourceRange>& Ranges,
                            ArrayRef<FixItHint> Hints);
 
diff --git a/clang/lib/Frontend/TextDiagnostic.cpp b/clang/lib/Frontend/TextDiagnostic.cpp
index 9bae0df..346443d 100644
--- a/clang/lib/Frontend/TextDiagnostic.cpp
+++ b/clang/lib/Frontend/TextDiagnostic.cpp
@@ -420,15 +420,8 @@ void TextDiagnostic::emitDiagnostic(SourceLocation Loc,
                          OS.tell() - StartOfLocationInfo,
                          DiagOpts.MessageLength, DiagOpts.ShowColors);
 
-  // If caret diagnostics are enabled and we have location, we want to
-  // emit the caret.  However, we only do this if the location moved
-  // from the last diagnostic, if the last diagnostic was a note that
-  // was part of a different warning or error diagnostic, or if the
-  // diagnostic has ranges.  We don't want to emit the same caret
-  // multiple times if one loc has multiple diagnostics.
-  if (DiagOpts.ShowCarets &&
-      (Loc != LastLoc || !Ranges.empty() || !FixItHints.empty() ||
-       (LastLevel == DiagnosticsEngine::Note && Level != LastLevel))) {
+  // Only recurse if we have a valid location.
+  if (Loc.isValid()) {
     // Get the ranges into a local array we can hack on.
     SmallVector<CharSourceRange, 20> MutableRanges(Ranges.begin(),
                                                    Ranges.end());
@@ -440,7 +433,8 @@ void TextDiagnostic::emitDiagnostic(SourceLocation Loc,
         MutableRanges.push_back(I->RemoveRange);
 
     unsigned MacroDepth = 0;
-    emitCaret(Loc, MutableRanges, FixItHints, MacroDepth);
+    emitMacroExpansionsAndCarets(Loc, Level, MutableRanges, FixItHints,
+                                 MacroDepth);
   }
 
   LastLoc = Loc;
@@ -653,26 +647,26 @@ void TextDiagnostic::emitDiagnosticLoc(SourceLocation Loc, PresumedLoc PLoc,
   OS << ' ';
 }
 
-/// \brief Emit the caret and underlining text.
-///
-/// Walks up the macro expansion stack printing the code snippet, caret,
-/// underlines and FixItHint display as appropriate at each level. Walk is
-/// accomplished by calling itself recursively.
+/// \brief Recursively emit notes for each macro expansion and caret
+/// diagnostics where appropriate.
 ///
-/// FIXME: Remove macro expansion from this routine, it shouldn't be tied to
-/// caret diagnostics.
-/// FIXME: Break up massive function into logical units.
+/// Walks up the macro expansion stack printing expansion notes, the code
+/// snippet, caret, underlines and FixItHint display as appropriate at each
+/// level.
 ///
 /// \param Loc The location for this caret.
+/// \param Level The diagnostic level currently being emitted.
 /// \param Ranges The underlined ranges for this code snippet.
 /// \param Hints The FixIt hints active for this diagnostic.
 /// \param MacroSkipEnd The depth to stop skipping macro expansions.
 /// \param OnMacroInst The current depth of the macro expansion stack.
-void TextDiagnostic::emitCaret(SourceLocation Loc,
-                               SmallVectorImpl<CharSourceRange>& Ranges,
-                               ArrayRef<FixItHint> Hints,
-                               unsigned &MacroDepth,
-                               unsigned OnMacroInst) {
+void TextDiagnostic::emitMacroExpansionsAndCarets(
+    SourceLocation Loc,
+    DiagnosticsEngine::Level Level,
+    SmallVectorImpl<CharSourceRange>& Ranges,
+    ArrayRef<FixItHint> Hints,
+    unsigned &MacroDepth,
+    unsigned OnMacroInst) {
   assert(!Loc.isInvalid() && "must have a valid source location here");
 
   // If this is a file source location, directly emit the source snippet and
@@ -680,7 +674,7 @@ void TextDiagnostic::emitCaret(SourceLocation Loc,
   if (Loc.isFileID()) {
     assert(MacroDepth == 0 && "We shouldn't hit a leaf node twice!");
     MacroDepth = OnMacroInst;
-    emitSnippetAndCaret(Loc, Ranges, Hints);
+    emitSnippetAndCaret(Loc, Level, Ranges, Hints);
     return;
   }
   // Otherwise recurse through each macro expansion layer.
@@ -692,7 +686,8 @@ void TextDiagnostic::emitCaret(SourceLocation Loc,
   SourceLocation OneLevelUp = getImmediateMacroCallerLoc(SM, Loc);
 
   // FIXME: Map ranges?
-  emitCaret(OneLevelUp, Ranges, Hints, MacroDepth, OnMacroInst + 1);
+  emitMacroExpansionsAndCarets(OneLevelUp, Level, Ranges, Hints, MacroDepth,
+                               OnMacroInst + 1);
 
   // Map the location.
   Loc = getImmediateMacroCalleeLoc(SM, Loc);
@@ -741,7 +736,8 @@ void TextDiagnostic::emitCaret(SourceLocation Loc,
     }
     OS << "note: expanded from:\n";
 
-    emitSnippetAndCaret(Loc, Ranges, ArrayRef<FixItHint>());
+    emitSnippetAndCaret(Loc, DiagnosticsEngine::Note, Ranges,
+                        ArrayRef<FixItHint>());
     return;
   }
 
@@ -761,12 +757,24 @@ void TextDiagnostic::emitCaret(SourceLocation Loc,
 /// \param Ranges The underlined ranges for this code snippet.
 /// \param Hints The FixIt hints active for this diagnostic.
 void TextDiagnostic::emitSnippetAndCaret(
-    SourceLocation Loc,
+    SourceLocation Loc, DiagnosticsEngine::Level Level,
     SmallVectorImpl<CharSourceRange>& Ranges,
     ArrayRef<FixItHint> Hints) {
   assert(!Loc.isInvalid() && "must have a valid source location here");
   assert(Loc.isFileID() && "must have a file location here");
 
+  // If caret diagnostics are enabled and we have location, we want to
+  // emit the caret.  However, we only do this if the location moved
+  // from the last diagnostic, if the last diagnostic was a note that
+  // was part of a different warning or error diagnostic, or if the
+  // diagnostic has ranges.  We don't want to emit the same caret
+  // multiple times if one loc has multiple diagnostics.
+  if (!DiagOpts.ShowCarets)
+    return;
+  if (Loc == LastLoc && Ranges.empty() && Hints.empty() &&
+      (LastLevel != DiagnosticsEngine::Note || Level == LastLevel))
+    return;
+
   // Decompose the location into a FID/Offset pair.
   std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Loc);
   FileID FID = LocInfo.first;
diff --git a/clang/test/Misc/macro-backtrace.c b/clang/test/Misc/macro-backtrace.c
index 819ec93..a76d4d2 100644
--- a/clang/test/Misc/macro-backtrace.c
+++ b/clang/test/Misc/macro-backtrace.c
@@ -31,4 +31,17 @@ void f(int *ip, float *fp) {
   // CHECK-LIMIT: #define M2(A, B) M1(A, B)
   // CHECK-LIMIT: macro-backtrace.c:4:23: note: expanded from:
   // CHECK-LIMIT: #define M1(A, B) ((A) < (B))
+
+  // FIXME: We should have higher quality messages, especially when caret
+  // diagnostics are off.
+  // RUN: %clang_cc1 -fsyntax-only -fno-caret-diagnostics %s 2>&1 \
+  // RUN:   | FileCheck %s -check-prefix=CHECK-NO-CARETS
+  // CHECK-NO-CARETS: macro-backtrace.c:18:7: warning: comparison of distinct pointer types ('int *' and 'float *')
+  // CHECK-NO-CARETS-NEXT: macro-backtrace.c:15:19: note: expanded from:
+  // CHECK-NO-CARETS-NEXT: macro-backtrace.c:14:19: note: expanded from:
+  // CHECK-NO-CARETS-NEXT: macro-backtrace.c:13:19: note: expanded from:
+  // CHECK-NO-CARETS-NEXT: note: (skipping 6 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
+  // CHECK-NO-CARETS-NEXT: macro-backtrace.c:6:18: note: expanded from:
+  // CHECK-NO-CARETS-NEXT: macro-backtrace.c:5:18: note: expanded from:
+  // CHECK-NO-CARETS-NEXT: macro-backtrace.c:4:23: note: expanded from:
 }