Looks great to me, thanks! - Daniel
On Sat, May 2, 2009 at 9:21 PM, Douglas Gregor <[email protected]> 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 <[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
