<div dir="ltr"><div class="gmail_default" style>On Fri, Jan 18, 2013 at 5:07 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br></div><div class="gmail_extra">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Fri, Jan 18, 2013 at 6:04 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>

> Author: klimek<br>
> Date: Fri Jan 18 08:04:34 2013<br>
> New Revision: 172819<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=172819&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=172819&view=rev</a><br>
> Log:<br>
> Fixes problems with line merging in the face of preprocessor directives.<br>
><br>
> This patch prepares being able to test for and fix more problems (see<br>
> FIXME in the test for example).<br>
><br>
> Previously we would output unwrapped lines for preprocessor directives<br>
> at the point where we also parsed the hash token. Since often<br>
> projections only terminate (and thus output their own unwrapped line)<br>
> after peeking at the next token, this would lead to the formatter seeing<br>
> the preprocessor directives out-of-order (slightly earlier). To be able<br>
> to correctly identify lines to merge, the formatter needs a well-defined<br>
> order of unwrapped lines, which this patch introduces.<br>
><br>
> Modified:<br>
>     cfe/trunk/lib/Format/UnwrappedLineParser.cpp<br>
>     cfe/trunk/lib/Format/UnwrappedLineParser.h<br>
>     cfe/trunk/unittests/Format/FormatTest.cpp<br>
><br>
> Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=172819&r1=172818&r2=172819&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=172819&r1=172818&r2=172819&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)<br>
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Fri Jan 18 08:04:34 2013<br>
> @@ -79,7 +79,11 @@<br>
><br>
>  class ScopedLineState {<br>
>  public:<br>
> -  ScopedLineState(UnwrappedLineParser &Parser) : Parser(Parser) {<br>
> +  ScopedLineState(UnwrappedLineParser &Parser,<br>
> +                  bool SwitchToPreprocessorLines = false)<br>
> +      : Parser(Parser), SwitchToPreprocessorLines(SwitchToPreprocessorLines) {<br>
> +    if (SwitchToPreprocessorLines)<br>
> +      Parser.CurrentLines = &Parser.PreprocessorDirectives;<br>
>      PreBlockLine = Parser.Line.take();<br>
>      Parser.Line.reset(new UnwrappedLine());<br>
>      Parser.Line->Level = PreBlockLine->Level;<br>
> @@ -93,10 +97,13 @@<br>
>      assert(Parser.Line->Tokens.empty());<br>
>      Parser.Line.reset(PreBlockLine);<br>
>      Parser.MustBreakBeforeNextToken = true;<br>
> +    if (SwitchToPreprocessorLines)<br>
> +      Parser.CurrentLines = &Parser.Lines;<br>
>    }<br>
><br>
>  private:<br>
>    UnwrappedLineParser &Parser;<br>
> +  const bool SwitchToPreprocessorLines;<br>
><br>
>    UnwrappedLine *PreBlockLine;<br>
>  };<br>
> @@ -104,14 +111,20 @@<br>
>  UnwrappedLineParser::UnwrappedLineParser(<br>
>      clang::DiagnosticsEngine &Diag, const FormatStyle &Style,<br>
>      FormatTokenSource &Tokens, UnwrappedLineConsumer &Callback)<br>
> -    : Line(new UnwrappedLine), MustBreakBeforeNextToken(false), Diag(Diag),<br>
> -      Style(Style), Tokens(&Tokens), Callback(Callback) {<br>
> -}<br>
> +    : Line(new UnwrappedLine), MustBreakBeforeNextToken(false),<br>
> +      CurrentLines(&Lines), Diag(Diag), Style(Style), Tokens(&Tokens),<br>
> +      Callback(Callback) {}<br>
><br>
>  bool UnwrappedLineParser::parse() {<br>
>    DEBUG(llvm::dbgs() << "----\n");<br>
>    readToken();<br>
> -  return parseFile();<br>
> +  bool Error = parseFile();<br>
> +  for (std::vector<UnwrappedLine>::iterator I = Lines.begin(),<br>
> +                                            E = Lines.end();<br>
> +       I != E; ++I) {<br>
> +    Callback.consumeUnwrappedLine(*I);<br>
> +  }<br>
> +  return Error;<br>
>  }<br>
><br>
>  bool UnwrappedLineParser::parseFile() {<br>
> @@ -668,8 +681,17 @@<br>
>      }<br>
>      llvm::dbgs() << "\n";<br>
>    });<br>
> -  Callback.consumeUnwrappedLine(*Line);<br>
> +  CurrentLines->push_back(*Line);<br>
>    Line->Tokens.clear();<br>
> +  if (CurrentLines == &Lines && !PreprocessorDirectives.empty()) {<br>
> +    for (std::vector<UnwrappedLine>::iterator I = PreprocessorDirectives<br>
> +             .begin(), E = PreprocessorDirectives.end();<br>
> +         I != E; ++I) {<br>
> +      CurrentLines->push_back(*I);<br>
> +    }<br>
> +    PreprocessorDirectives.clear();<br>
> +  }<br>
> +<br>
>  }<br>
><br>
>  bool UnwrappedLineParser::eof() const {<br>
> @@ -692,7 +714,11 @@<br>
>    while (!Line->InPPDirective && <a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::hash) &&<br>
>           ((FormatTok.NewlinesBefore > 0 && FormatTok.HasUnescapedNewline) ||<br>
>            FormatTok.IsFirst)) {<br>
> -    ScopedLineState BlockState(*this);<br>
> +    // If there is an unfinished unwrapped line, we flush the preprocessor<br>
> +    // directives only after that unwrapped line was finished later.<br>
> +    bool SwitchToPreprocessorLines = !Line->Tokens.empty() &&<br>
> +                                     CurrentLines == &Lines;<br>
> +    ScopedLineState BlockState(*this, SwitchToPreprocessorLines);<br>
>      parsePPDirective();<br>
>    }<br>
>  }<br>
><br>
> Modified: cfe/trunk/lib/Format/UnwrappedLineParser.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=172819&r1=172818&r2=172819&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=172819&r1=172818&r2=172819&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/lib/Format/UnwrappedLineParser.h (original)<br>
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.h Fri Jan 18 08:04:34 2013<br>
> @@ -159,6 +159,21 @@<br>
>    FormatToken FormatTok;<br>
>    bool MustBreakBeforeNextToken;<br>
><br>
> +  // The parsed lines. This is a pointer<br>
<br>
</div></div>Doesn't look like a pointer.<br></blockquote><div><br></div><div style>Design changed, comment didnt :) Fixed in r172831. Thx for spotting.</div><div style><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<div class=""><div class="h5"><br>
> so we can switch it out to parse an<br>
> +  // unrelated set of unwrapped lines and put them into place later.<br>
> +  std::vector<UnwrappedLine> Lines;<br>
> +<br>
> +  // Preprocessor directives are parsed out-of-order from other unwrapped lines.<br>
> +  // Thus, we need to keep a list of preprocessor directives to be reported<br>
> +  // after an unwarpped line that has been started was finished.<br>
> +  std::vector<UnwrappedLine> PreprocessorDirectives;<br>
> +<br>
> +  // New unwrapped lines are added via CurrentLines.<br>
> +  // Usually points to \c &Lines. While parsing a preprocessor directive when<br>
> +  // there is an unfinished previous unwrapped line, will point to<br>
> +  // \c &PreprocessorDirectives.<br>
> +  std::vector<UnwrappedLine> *CurrentLines;<br>
> +<br>
>    clang::DiagnosticsEngine &Diag;<br>
>    const FormatStyle &Style;<br>
>    FormatTokenSource *Tokens;<br>
><br>
> Modified: cfe/trunk/unittests/Format/FormatTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=172819&r1=172818&r2=172819&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=172819&r1=172818&r2=172819&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Fri Jan 18 08:04:34 2013<br>
> @@ -1537,6 +1537,16 @@<br>
>    EXPECT_EQ("#warning 1", format("  #  warning 1"));<br>
>  }<br>
><br>
> +TEST_F(FormatTest, MergeHandlingInTheFaceOfPreprocessorDirectives) {<br>
> +  FormatStyle AllowsMergedIf = getGoogleStyle();<br>
> +  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;<br>
> +  verifyFormat("void f() { f(); }\n#error E", AllowsMergedIf);<br>
> +  verifyFormat("if (true) return 42;\n#error E", AllowsMergedIf);<br>
> +<br>
> +  // FIXME:<br>
> +  // verifyFormat("if (true)\n#error E\n  return 42;", AllowsMergedIf);<br>
> +}<br>
> +<br>
>  // FIXME: This breaks the order of the unwrapped lines:<br>
>  // TEST_F(FormatTest, OrderUnwrappedLines) {<br>
>  //   verifyFormat("{\n"<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>