[cfe-commits] r70597 - in /cfe/trunk: include/clang/Frontend/TextDiagnosticPrinter.h lib/Frontend/TextDiagnosticPrinter.cpp test/Misc/message-length.c
Daniel Dunbar
daniel at zuster.org
Sun May 3 13:21:26 PDT 2009
Looks great to me, thanks!
- Daniel
On Sat, May 2, 2009 at 9:21 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> 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/20090503/6620cf68/attachment.html>
More information about the cfe-commits
mailing list