[cfe-commits] r70597 - in /cfe/trunk: include/clang/Frontend/TextDiagnosticPrinter.h lib/Frontend/TextDiagnosticPrinter.cpp test/Misc/message-length.c
Douglas Gregor
dgregor at apple.com
Sat May 2 21:21:25 PDT 2009
On May 2, 2009, at 11:46 AM, Daniel Dunbar wrote:
> 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?
I've done this... it looks okay to me. Et tu?
- Doug
> On Fri, May 1, 2009 at 4:32 PM, Douglas Gregor <dgregor at apple.com>
> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090502/86d5252d/attachment.html>
More information about the cfe-commits
mailing list