[clang][Diagnostics] Split source ranges into line ranges before...

[tbaeder]

Summary

... emitting them.

This makes later code easier to understand, since we emit the code
snippets line by line anyway.
It also fixes the weird underlinig of multi-line source ranges.

[aaron.ballman]

This looks correct to me, but because it involves things like characters, lines, and output, I'd appreciate if @tahonermann or @cor3ntin could do the final sign-off just in case I missed something.

[cor3ntin]

Looks good to me too!

[tbaeder]

@cor3ntin I added changes to the two brackets tests in test/Parser/. They look like an improvement to me, but maybe there's something I'm missing, so please have a quick look.

clang/lib/Frontend/TextDiagnostic.cpp

maybeAddRange(std::pair<unsigned, unsigned> A, std::pair<unsigned, unsigned> B,
   // much as possible, then downwards as much as possible.
   A.second = std::min(A.second + (Slack + 1) / 2, Max);
   Slack = MaxRange - (A.second - A.first + 1);
   A.first = std::max(Min + Slack, A.first) - Slack;
   A.second = std::min(A.first + MaxRange - 1, Max);
   return A;
 }
 
-/// Highlight a SourceRange (with ~'s) for any characters on LineNo.
-static void highlightRange(const CharSourceRange &R,
-                           unsigned LineNo, FileID FID,
-                           const SourceColumnMap &map,
-                           std::string &CaretLine,
-                           const SourceManager &SM,
-                           const LangOptions &LangOpts) {
-  if (!R.isValid()) return;
-
-  SourceLocation Begin = R.getBegin();
-  SourceLocation End = R.getEnd();
-
-  unsigned StartLineNo = SM.getExpansionLineNumber(Begin);
-  if (StartLineNo > LineNo || SM.getFileID(Begin) != FID)
-    return;  // No intersection.
-
-  unsigned EndLineNo = SM.getExpansionLineNumber(End);
-  if (EndLineNo < LineNo || SM.getFileID(End) != FID)
-    return;  // No intersection.
-
-  // Compute the column number of the start.
-  unsigned StartColNo = 0;
-  if (StartLineNo == LineNo) {
-    StartColNo = SM.getExpansionColumnNumber(Begin);
-    if (StartColNo) --StartColNo;  // Zero base the col #.
-  }
-
-  // Compute the column number of the end.
-  unsigned EndColNo = map.getSourceLine().size();
-  if (EndLineNo == LineNo) {
-    EndColNo = SM.getExpansionColumnNumber(End);
-    if (EndColNo) {
-      --EndColNo;  // Zero base the col #.
-
-      // Add in the length of the token, so that we cover multi-char tokens if
-      // this is a token range.
-      if (R.isTokenRange())
-        EndColNo += Lexer::MeasureTokenLength(End, SM, LangOpts);
-    } else {
-      EndColNo = CaretLine.size();
-    }
-  }
-
-  assert(StartColNo <= EndColNo && "Invalid range!");
+struct LineRange {
+  unsigned LineNo;
+  unsigned StartCol;
+  unsigned EndCol;
+};
 
-  // Check that a token range does not highlight only whitespace.
-  if (R.isTokenRange()) {
-    // Pick the first non-whitespace column.
-    while (StartColNo < map.getSourceLine().size() &&
-           (map.getSourceLine()[StartColNo] == ' ' ||
-            map.getSourceLine()[StartColNo] == '\t'))
-      StartColNo = map.startOfNextColumn(StartColNo);
+/// Highlight \p R (with ~'s) on the current source line.
+static void highlightRange(const LineRange &R, const SourceColumnMap &Map,
+                           std::string &CaretLine) {
+  unsigned StartColNo = R.StartCol;
+  while (StartColNo < Map.getSourceLine().size() &&
+         (Map.getSourceLine()[StartColNo] == ' ' ||
+          Map.getSourceLine()[StartColNo] == '\t'))
+    StartColNo = Map.startOfNextColumn(StartColNo);
 
-    // Pick the last non-whitespace column.
-    if (EndColNo > map.getSourceLine().size())
-      EndColNo = map.getSourceLine().size();
-    while (EndColNo &&
-           (map.getSourceLine()[EndColNo-1] == ' ' ||
-            map.getSourceLine()[EndColNo-1] == '\t'))
-      EndColNo = map.startOfPreviousColumn(EndColNo);
+  unsigned EndColNo =
+      std::min(static_cast<size_t>(R.EndCol), Map.getSourceLine().size());
+  while (EndColNo && (Map.getSourceLine()[EndColNo - 1] == ' ' ||
+                      Map.getSourceLine()[EndColNo - 1] == '\t'))
+    EndColNo = Map.startOfPreviousColumn(EndColNo);
 
-    // If the start/end passed each other, then we are trying to highlight a
-    // range that just exists in whitespace. That most likely means we have
-    // a multi-line highlighting range that covers a blank line.
-    if (StartColNo > EndColNo) {
-      assert(StartLineNo != EndLineNo && "trying to highlight whitespace");
-      StartColNo = EndColNo;
-    }
-  }
-
-  assert(StartColNo <= map.getSourceLine().size() && "Invalid range!");
-  assert(EndColNo <= map.getSourceLine().size() && "Invalid range!");
+  if (StartColNo > EndColNo)
+    return;
 
   // Fill the range with ~'s.
-  StartColNo = map.byteToContainingColumn(StartColNo);
-  EndColNo = map.byteToContainingColumn(EndColNo);
+  StartColNo = Map.byteToContainingColumn(StartColNo);
+  EndColNo = Map.byteToContainingColumn(EndColNo);
 
   assert(StartColNo <= EndColNo && "Invalid range!");
   if (CaretLine.size() < EndColNo)
-    CaretLine.resize(EndColNo,' ');
-  std::fill(CaretLine.begin()+StartColNo,CaretLine.begin()+EndColNo,'~');
+    CaretLine.resize(EndColNo, ' ');
+  std::fill(CaretLine.begin() + StartColNo, CaretLine.begin() + EndColNo, '~');
 }
 
 static std::string buildFixItInsertionLine(FileID FID,
                                            unsigned LineNo,
                                            const SourceColumnMap &map,
                                            ArrayRef<FixItHint> Hints,
                                            const SourceManager &SM,
                                            const DiagnosticOptions *DiagOpts) {

 static unsigned getNumDisplayWidth(unsigned N) {
   unsigned L = 1u, M = 10u;
   while (M <= N && ++L != std::numeric_limits<unsigned>::digits10 + 1)
     M *= 10u;
 
   return L;
 }
 
+/// Filter out invalid ranges, ranges that don't fit into the window of
+/// source lines we will print, and ranges from other files.
+///
+/// For the remaining ranges, convert them to simple LineRange structs,
+/// which only cover one line at a time.
+static SmallVector<LineRange>
+prepareAndFilterRanges(const SmallVectorImpl<CharSourceRange> &Ranges,
+                       const SourceManager &SM,
+                       const std::pair<unsigned, unsigned> &Lines, FileID FID,
+                       const LangOptions &LangOpts) {
+  SmallVector<LineRange> LineRanges;
+
+  for (const CharSourceRange &R : Ranges) {
+    if (R.isInvalid())
+      continue;
+    SourceLocation Begin = R.getBegin();
+    SourceLocation End = R.getEnd();
+
+    unsigned StartLineNo = SM.getExpansionLineNumber(Begin);
+    if (StartLineNo > Lines.second || SM.getFileID(Begin) != FID)
+      continue;
+
+    unsigned EndLineNo = SM.getExpansionLineNumber(End);
+    if (EndLineNo < Lines.first || SM.getFileID(End) != FID)
+      continue;
+
+    unsigned StartColumn = SM.getExpansionColumnNumber(Begin);
+    unsigned EndColumn = SM.getExpansionColumnNumber(End);
+    if (R.isTokenRange())
+      EndColumn += Lexer::MeasureTokenLength(End, SM, LangOpts);
+
+    // Only a single line.
+    if (StartLineNo == EndLineNo) {
+      LineRanges.push_back({StartLineNo, StartColumn - 1, EndColumn - 1});
+      continue;
+    }
+
+    // Start line.
+    LineRanges.push_back({StartLineNo, StartColumn - 1, ~0u});
+
+    // Middle lines.
+    for (unsigned S = StartLineNo + 1; S != EndLineNo; ++S)
+      LineRanges.push_back({S, 0, ~0u});
+
+    // End line.
+    LineRanges.push_back({EndLineNo, 0, EndColumn - 1});
+  }
+
+  return LineRanges;
+}
+
 /// Emit a code snippet and caret line.
 ///
 /// This routine emits a single line's code snippet and caret line..
 ///
 /// \param Loc The location for the caret.
 /// \param Ranges The underlined ranges for this code snippet.
 /// \param Hints The FixIt hints active for this diagnostic.
 void TextDiagnostic::emitSnippetAndCaret(
unsigned MaxLineNoDisplayWidth =
       DiagOpts->ShowLineNumbers
           ? std::max(4u, getNumDisplayWidth(DisplayLineNo + MaxLines))
           : 0;
   auto indentForLineNumbers = [&] {
     if (MaxLineNoDisplayWidth > 0)
       OS.indent(MaxLineNoDisplayWidth + 2) << "| ";
   };
 
+  SmallVector<LineRange> LineRanges =
+      prepareAndFilterRanges(Ranges, SM, Lines, FID, LangOpts);
+
   for (unsigned LineNo = Lines.first; LineNo != Lines.second + 1;
        ++LineNo, ++DisplayLineNo) {
     // Rewind from the current position to the start of the line.
     const char *LineStart =
         BufStart +
         SM.getDecomposedLoc(SM.translateLineCol(FID, LineNo, 1)).second;
     if (LineStart == BufEnd)
       break;
while (!SourceLine.empty() && SourceLine.back() == '\0' &&
            (LineNo != CaretLineNo || SourceLine.size() > CaretColNo))
       SourceLine.pop_back();
 
     // Build the byte to column map.
     const SourceColumnMap sourceColMap(SourceLine, DiagOpts->TabStop);
 
     std::string CaretLine;
     // Highlight all of the characters covered by Ranges with ~ characters.
-    for (const auto &I : Ranges)
-      highlightRange(I, LineNo, FID, sourceColMap, CaretLine, SM, LangOpts);
+    for (const auto &LR : LineRanges) {
+      if (LR.LineNo == LineNo)
+        highlightRange(LR, sourceColMap, CaretLine);
+    }
 
     // Next, insert the caret itself.
     if (CaretLineNo == LineNo) {
       size_t Col = sourceColMap.byteToContainingColumn(CaretColNo - 1);
       CaretLine.resize(std::max(Col + 1, CaretLine.size()), ' ');
       CaretLine[Col] = '^';
     }
 

clang/test/Misc/caret-diags-multiline.cpp

 // RUN: not %clang_cc1 -std=c++11 -fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines 5 -Wsometimes-uninitialized %s 2>&1 | FileCheck %s --strict-whitespace
 
 void line(int);
 
 // Check we expand the range as much as possible within the limit.
 
 // CHECK:      warning: variable 'a' is used uninitialized whenever 'if' condition is true
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}      ^~~~{{$}}
 // CHECK-NEXT: note: uninitialized use occurs here
 // CHECK-NEXT: {{^}}  return a;
 // CHECK-NEXT: {{^}}         ^
 // CHECK-NEXT: note: remove the 'if' if its condition is always false
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}  ^~~~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(1);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}  } else {
-// CHECK-NEXT: {{^}}~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}  ~~~~~~{{$}}
 // CHECK-NEXT: note: initialize the variable
 int f1(int cond) {
   int a;
   if (cond) {
     line(1);
   } else {
     a = 3;
   }
   return a;
 }
 
 // CHECK:      warning: variable 'a' is used uninitialized whenever 'if' condition is true
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}      ^~~~{{$}}
 // CHECK-NEXT: note: uninitialized use occurs here
 // CHECK-NEXT: {{^}}  return a;
 // CHECK-NEXT: {{^}}         ^
 // CHECK-NEXT: note: remove the 'if' if its condition is always false
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}  ^~~~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(1);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(2);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}  } else {
-// CHECK-NEXT: {{^}}~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}  ~~~~~~{{$}}
 // CHECK-NEXT: note: initialize the variable
 int f2(int cond) {
   int a;
   if (cond) {
     line(1);
     line(2);
   } else {
     a = 3;
   }
   return a;
 }
 
 // CHECK:      warning: variable 'a' is used uninitialized whenever 'if' condition is true
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}      ^~~~{{$}}
 // CHECK-NEXT: note: uninitialized use occurs here
 // CHECK-NEXT: {{^}}  return a;
 // CHECK-NEXT: {{^}}         ^
 // CHECK-NEXT: note: remove the 'if' if its condition is always false
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}  ^~~~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(1);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(2);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(3);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}  } else {
-// CHECK-NEXT: {{^}}~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}  ~~~~~~{{$}}
 // CHECK-NEXT: note: initialize the variable
 int f3(int cond) {
   int a;
   if (cond) {
     line(1);
     line(2);
     line(3);
   } else {
     a = 3;
   }
   return a;
 }
 
 // CHECK:      warning: variable 'a' is used uninitialized whenever 'if' condition is true
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}      ^~~~{{$}}
 // CHECK-NEXT: note: uninitialized use occurs here
 // CHECK-NEXT: {{^}}  return a;
 // CHECK-NEXT: {{^}}         ^
 // CHECK-NEXT: note: remove the 'if' if its condition is always false
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}  ^~~~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(1);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(2);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(3);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(4);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: note: initialize the variable
 int f4(int cond) {
   int a;
   if (cond) {
     line(1);
     line(2);
     line(3);
     line(4);
   } else {
     a = 3;
   }
   return a;
 }
 
 // CHECK:      warning: variable 'a' is used uninitialized whenever 'if' condition is true
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}      ^~~~{{$}}
 // CHECK-NEXT: note: uninitialized use occurs here
 // CHECK-NEXT: {{^}}  return a;
 // CHECK-NEXT: {{^}}         ^
 // CHECK-NEXT: note: remove the 'if' if its condition is always false
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}  ^~~~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(1);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(2);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(3);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: {{^}}    line(4);
-// CHECK-NEXT: {{^}}~~~~~~~~~~~~{{$}}
+// CHECK-NEXT: {{^}}    ~~~~~~~~{{$}}
 // CHECK-NEXT: note: initialize the variable
 int f5(int cond) {
   int a;
   if (cond) {
     line(1);
     line(2);
     line(3);
     line(4);