Very nice! One comment though, it seems like it might be nice to give some indication that the line is truncated. Leading and trailing ellipsis, perhaps?
- Daniel On Fri, May 1, 2009 at 4:32 PM, Douglas Gregor <[email protected]> wrote: > Author: dgregor > Date: Fri May 1 18:32:58 2009 > New Revision: 70597 > > URL: http://llvm.org/viewvc/llvm-project?rev=70597&view=rev > Log: > When printing a source line as part of a diagnostic, the source line > might be wider than we're supposed to print. In this case, we try to > select the "important" subregion of the source line, which contains > everything that we want to show (e.g., with underlining and the caret > itself) and tries to also contain some of the context. > > >From the fantastically long line in the test case, we get an error > message that slices down to this: > > message-length.c:18:120: warning: comparison of distinct pointer types > ('int *' and 'float *') > a_func_to_call(ip == FloatPointer, ip[ALongIndexName], > ~~ ^ ~~~~~~~~~~~~ > > There are a bunch of gee-it-sounds-good heuristics in here, which seem > to do well on the various simple tests I've thrown at it. However, > we're going to need to look at a bunch more diagnostics to tweak these > heuristics. > > This is the second part of <rdar://problem/6711348>. Almost there! > > > Modified: > cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h > cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp > cfe/trunk/test/Misc/message-length.c > > Modified: cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h?rev=70597&r1=70596&r2=70597&view=diff > > > ============================================================================== > --- cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h (original) > +++ cfe/trunk/include/clang/Frontend/TextDiagnosticPrinter.h Fri May 1 > 18:32:58 2009 > @@ -74,7 +74,8 @@ > SourceManager &SM, > const CodeModificationHint *Hints, > unsigned NumHints, > - unsigned AvoidColumn); > + unsigned AvoidColumn, > + unsigned Columns); > > virtual void HandleDiagnostic(Diagnostic::Level DiagLevel, > const DiagnosticInfo &Info); > > Modified: cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp?rev=70597&r1=70596&r2=70597&view=diff > > > ============================================================================== > --- cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp (original) > +++ cfe/trunk/lib/Frontend/TextDiagnosticPrinter.cpp Fri May 1 18:32:58 > 2009 > @@ -108,20 +108,158 @@ > CaretLine[i] = '~'; > } > > +/// \brief Whether this is a closing delimiter such as ')' or ']'. > +static inline bool isClosingDelimiter(char c) { > + return c == ')' || c == ']' || c == '}'; > +} > + > +/// \brief When the source code line we want to print is too long for > +/// the terminal, select the "interesting" region. > +static void SelectInterestingSourceRegion(std::string &SourceLine, > + std::string &CaretLine, > + std::string &FixItInsertionLine, > + unsigned Columns) { > + if (CaretLine.size() > SourceLine.size()) > + SourceLine.resize(CaretLine.size(), ' '); > + > + // Find the slice that we need to display the full caret line > + // correctly. > + unsigned CaretStart = 0, CaretEnd = CaretLine.size(); > + for (; CaretStart != CaretEnd; ++CaretStart) > + if (!isspace(CaretLine[CaretStart])) > + break; > + > + for (; CaretEnd != CaretStart; --CaretEnd) > + if (!isspace(CaretLine[CaretEnd - 1])) > + break; > + > + // CaretLine[CaretStart, CaretEnd) contains all of the interesting > + // parts of the caret line. While this slice is smaller than the > + // number of columns we have, try to grow the slice to encompass > + // more context. > + > + // If the end of the interesting region comes before we run out of > + // space in the terminal, start at the beginning of the line. > + if (CaretEnd < Columns) > + CaretStart = 0; > + > + unsigned TargetColumns = Columns - 4; // Give us a little extra room. > + unsigned SourceLength = SourceLine.size(); > + bool StartIsFixed = false; > + while (CaretEnd - CaretStart < TargetColumns) { > + bool ExpandedRegion = false; > + // Move the start of the interesting region left until we've > + // pulled in something else interesting. > + if (CaretStart && !StartIsFixed && > + CaretEnd - CaretStart < TargetColumns) { > + unsigned NewStart = CaretStart; > + > + bool BadStart = false; > + do { > + // Skip over any whitespace we see here; we're looking for > + // another bit of interesting text. > + if (NewStart) > + --NewStart; > + while (NewStart && isspace(SourceLine[NewStart])) > + --NewStart; > + > + // Skip over this bit of "interesting" text. > + while (NewStart && !isspace(SourceLine[NewStart])) { > + if (isClosingDelimiter(SourceLine[NewStart])) > + StartIsFixed = true; > + --NewStart; > + } > + > + // Move up to the non-whitespace character we just saw. > + if (!StartIsFixed && > + isspace(SourceLine[NewStart]) && > + !isspace(SourceLine[NewStart + 1])) > + ++NewStart; > + > + // Never go back past closing delimeters, because > + // they're unlikely to be important (and they result in > + // weird slices). Instead, move forward to the next > + // non-whitespace character. > + BadStart = false; > + if (StartIsFixed) { > + ++NewStart; > + while (NewStart != CaretEnd && isspace(SourceLine[NewStart])) > + ++NewStart; > + } else if (NewStart) { > + // There are some characters that always signal that we've > + // found a bad stopping place, because they always occur in > + // the middle of or at the end of an expression. In these > + // cases, we either keep bringing in more "interesting" text > + // to try to get to a somewhat-complete slice of the code. > + BadStart = ispunct(SourceLine[NewStart]); > + } > + } while (BadStart); > + > + // If we're still within our limit, update the starting > + // position within the source/caret line. > + if (CaretEnd - NewStart <= TargetColumns && !StartIsFixed) { > + CaretStart = NewStart; > + ExpandedRegion = true; > + } > + } > + > + // Move the end of the interesting region right until we've > + // pulled in something else interesting. > + if (CaretEnd != SourceLength && > + CaretEnd - CaretStart < TargetColumns) { > + unsigned NewEnd = CaretEnd; > + > + // Skip over any whitespace we see here; we're looking for > + // another bit of interesting text. > + while (CaretEnd != SourceLength && isspace(SourceLine[NewEnd - 1])) > + ++NewEnd; > + > + // Skip over this bit of "interesting" text. > + while (CaretEnd != SourceLength && !isspace(SourceLine[NewEnd - 1])) > + ++NewEnd; > + > + if (NewEnd - CaretStart <= TargetColumns) { > + CaretEnd = NewEnd; > + ExpandedRegion = true; > + } > + > + if (!ExpandedRegion) > + break; > + } > + } > + > + // [CaretStart, CaretEnd) is the slice we want. Update the various > + // output lines to show only this slice, with two-space padding > + // before the lines so that it looks nicer. > + SourceLine.erase(CaretEnd, std::string::npos); > + CaretLine.erase(CaretEnd, std::string::npos); > + if (FixItInsertionLine.size() > CaretEnd) > + FixItInsertionLine.erase(CaretEnd, std::string::npos); > + > + if (CaretStart > 2) { > + SourceLine.replace(0, CaretStart, " "); > + CaretLine.replace(0, CaretStart, " "); > + if (FixItInsertionLine.size() >= CaretStart) > + FixItInsertionLine.replace(0, CaretStart, " "); > + } > +} > + > void TextDiagnosticPrinter::EmitCaretDiagnostic(SourceLocation Loc, > SourceRange *Ranges, > unsigned NumRanges, > SourceManager &SM, > const CodeModificationHint > *Hints, > unsigned NumHints, > - unsigned AvoidColumn) { > + unsigned AvoidColumn, > + unsigned Columns) { > assert(!Loc.isInvalid() && "must have a valid source location here"); > > // We always emit diagnostics about the instantiation points, not the > spelling > // points. This more closely correlates to what the user writes. > if (!Loc.isFileID()) { > SourceLocation OneLevelUp = > SM.getImmediateInstantiationRange(Loc).first; > - EmitCaretDiagnostic(OneLevelUp, Ranges, NumRanges, SM, 0, 0, > AvoidColumn); > + EmitCaretDiagnostic(OneLevelUp, Ranges, NumRanges, SM, 0, 0, > AvoidColumn, > + Columns); > > // Map the location through the macro. > Loc = SM.getInstantiationLoc(SM.getImmediateSpellingLoc(Loc)); > @@ -210,10 +348,6 @@ > CaretLine.insert(i+1, NumSpaces, CaretLine[i] == '~' ? '~' : ' '); > } > > - // Finally, remove any blank spaces from the end of CaretLine. > - while (CaretLine[CaretLine.size()-1] == ' ') > - CaretLine.erase(CaretLine.end()-1); > - > // If we are in -fdiagnostics-print-source-range-info mode, we are trying > to > // produce easily machine parsable output. Add a space before the source > line > // and the caret to make it trivial to tell the main diagnostic line from > what > @@ -222,27 +356,9 @@ > SourceLine = ' ' + SourceLine; > CaretLine = ' ' + CaretLine; > } > - > - // AvoidColumn tells us which column we should avoid when printing > - // the source line. If the source line would start at or near that > - // column, add another line of whitespace before printing the source > - // line. Otherwise, the source line and the diagnostic text can get > - // jumbled together. > - unsigned StartCol = 0; > - for (unsigned N = SourceLine.size(); StartCol != N; ++StartCol) > - if (!isspace(SourceLine[StartCol])) > - break; > - > - if (StartCol != SourceLine.size() && > - abs((int)StartCol - (int)AvoidColumn) <= 2) > - OS << '\n'; > - > - // Emit what we have computed. > - OS << SourceLine << '\n'; > - OS << CaretLine << '\n'; > - > + > + std::string FixItInsertionLine; > if (NumHints && PrintFixItInfo) { > - std::string InsertionLine; > for (const CodeModificationHint *Hint = Hints, *LastHint = Hints + > NumHints; > Hint != LastHint; ++Hint) { > if (Hint->InsertionLoc.isValid()) { > @@ -258,19 +374,47 @@ > = SM.getColumnNumber(HintLocInfo.first, HintLocInfo.second); > unsigned LastColumnModified > = HintColNo - 1 + Hint->CodeToInsert.size(); > - if (LastColumnModified > InsertionLine.size()) > - InsertionLine.resize(LastColumnModified, ' '); > + if (LastColumnModified > FixItInsertionLine.size()) > + FixItInsertionLine.resize(LastColumnModified, ' '); > std::copy(Hint->CodeToInsert.begin(), Hint->CodeToInsert.end(), > - InsertionLine.begin() + HintColNo - 1); > + FixItInsertionLine.begin() + HintColNo - 1); > } > } > } > + } > > - if (!InsertionLine.empty()) { > - if (PrintRangeInfo) > - OS << ' '; > - OS << InsertionLine << '\n'; > - } > + // If the source line is too long for our terminal, select only the > + // "interesting" source region within that line. > + if (Columns && SourceLine.size() > Columns) > + SelectInterestingSourceRegion(SourceLine, CaretLine, > FixItInsertionLine, > + Columns); > + > + // AvoidColumn tells us which column we should avoid when printing > + // the source line. If the source line would start at or near that > + // column, add another line of whitespace before printing the source > + // line. Otherwise, the source line and the diagnostic text can get > + // jumbled together. > + unsigned StartCol = 0; > + for (unsigned N = SourceLine.size(); StartCol != N; ++StartCol) > + if (!isspace(SourceLine[StartCol])) > + break; > + > + if (StartCol != SourceLine.size() && > + abs((int)StartCol - (int)AvoidColumn) <= 2) > + OS << '\n'; > + > + // Finally, remove any blank spaces from the end of CaretLine. > + while (CaretLine[CaretLine.size()-1] == ' ') > + CaretLine.erase(CaretLine.end()-1); > + > + // Emit what we have computed. > + OS << SourceLine << '\n'; > + OS << CaretLine << '\n'; > + > + if (!FixItInsertionLine.empty()) { > + if (PrintRangeInfo) > + OS << ' '; > + OS << FixItInsertionLine << '\n'; > } > } > > @@ -572,7 +716,8 @@ > EmitCaretDiagnostic(LastLoc, Ranges, NumRanges, LastLoc.getManager(), > Info.getCodeModificationHints(), > Info.getNumCodeModificationHints(), > - WordWrapped? WordWrapIndentation : 0); > + WordWrapped? WordWrapIndentation : 0, > + MessageLength); > } > > OS.flush(); > > Modified: cfe/trunk/test/Misc/message-length.c > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/message-length.c?rev=70597&r1=70596&r2=70597&view=diff > > > ============================================================================== > --- cfe/trunk/test/Misc/message-length.c (original) > +++ cfe/trunk/test/Misc/message-length.c Fri May 1 18:32:58 2009 > @@ -11,3 +11,12 @@ > > int (*fp2)(int, float, short, float) = f; > } > + > +void a_func_to_call(int, int, int); > + > +void a_very_long_line(int *ip, float *FloatPointer) { > + for (int ALongIndexName = 0; ALongIndexName < 100; ALongIndexName++) if > (ip[ALongIndexName] == 17) a_func_to_call(ip == FloatPointer, > ip[ALongIndexName], FloatPointer[ALongIndexName]); > + > + > + int array0[] = { [3] 3, 5, 7, 4, 2, 7, 6, 3, 4, 5, 6, 7, 8, 9, 12, 345, > 14, 345, 789, 234, 678, 345, 123, 765, 234 }; > +} > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
