<div dir="ltr"><div class="gmail_default" style>On Wed, Jan 9, 2013 at 5:18 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Wed, Jan 9, 2013 at 7:25 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>

> Author: klimek<br>
> Date: Wed Jan  9 09:25:02 2013<br>
> New Revision: 171974<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=171974&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=171974&view=rev</a><br>
> Log:<br>
> Enables layouting unwrapped lines around preprocessor directives.<br>
><br>
> Previously, we'd always start at indent level 0 after a preprocessor<br>
> directive, now we layout the following snippet (column limit 69) as<br>
> follows:<br>
><br>
> functionCallTo(someOtherFunction(<br>
>     withSomeParameters, whichInSequence,<br>
>     areLongerThanALine(andAnotherCall,<br>
>   B<br>
>                        withMoreParamters,<br>
>                        whichStronglyInfluenceTheLayout),<br>
>     andMoreParameters),<br>
>                trailing);<br>
><br>
> Note that the different jumping indent is a different issue that will be<br>
> addressed separately.<br>
><br>
> This is the first step towards handling #ifdef->#else->#endif chains<br>
> correctly.<br>
<br>
</div>Cool! Is there a tracking bug for #ifdef chains?<br></blockquote><div><br></div><div style>Nope. If you have opinions on how certain things should be formatted, or what we should prioritize, please file bugs :D</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
> Modified:<br>
>     cfe/trunk/lib/Format/Format.cpp<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/Format.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=171974&r1=171973&r2=171974&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=171974&r1=171973&r2=171974&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/lib/Format/Format.cpp (original)<br>
> +++ cfe/trunk/lib/Format/Format.cpp Wed Jan  9 09:25:02 2013<br>
> @@ -803,18 +803,21 @@<br>
>    void calculateExtraInformation(AnnotatedToken &Current) {<br>
>      Current.SpaceRequiredBefore = spaceRequiredBefore(Current);<br>
><br>
> -    if (Current.Type == TT_CtorInitializerColon || Current.Parent->Type ==<br>
> -        TT_LineComment || (Current.is(tok::string_literal) &&<br>
> -                           Current.Parent->is(tok::string_literal))) {<br>
> -      Current.MustBreakBefore = true;<br>
> -    } else if (Current.is(tok::at) && Current.Parent->Parent->is(tok::at)) {<br>
> -      // Don't put two objc's '@' on the same line. This could happen,<br>
> -      // as in, @optional @property ...<br>
> +    if (Current.FormatTok.MustBreakBefore) {<br>
>        Current.MustBreakBefore = true;<br>
>      } else {<br>
> -      Current.MustBreakBefore = false;<br>
> +      if (Current.Type == TT_CtorInitializerColon || Current.Parent->Type ==<br>
> +          TT_LineComment || (Current.is(tok::string_literal) &&<br>
> +                             Current.Parent->is(tok::string_literal))) {<br>
> +        Current.MustBreakBefore = true;<br>
> +      } else if (Current.is(tok::at) && Current.Parent->Parent->is(tok::at)) {<br>
> +        // Don't put two objc's '@' on the same line. This could happen,<br>
> +        // as in, @optional @property ...<br>
> +        Current.MustBreakBefore = true;<br>
> +      } else {<br>
> +        Current.MustBreakBefore = false;<br>
> +      }<br>
>      }<br>
> -<br>
>      Current.CanBreakBefore = Current.MustBreakBefore || canBreakBefore(Current);<br>
><br>
>      if (!Current.Children.empty())<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=171974&r1=171973&r2=171974&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=171974&r1=171973&r2=171974&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)<br>
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Wed Jan  9 09:25:02 2013<br>
> @@ -74,8 +74,9 @@<br>
>  UnwrappedLineParser::UnwrappedLineParser(const FormatStyle &Style,<br>
>                                           FormatTokenSource &Tokens,<br>
>                                           UnwrappedLineConsumer &Callback)<br>
> -    : RootTokenInitialized(false), Style(Style), Tokens(&Tokens),<br>
> -      Callback(Callback) {<br>
> +    : Line(new UnwrappedLine), RootTokenInitialized(false),<br>
> +      LastInCurrentLine(NULL), MustBreakBeforeNextToken(false), Style(Style),<br>
> +      Tokens(&Tokens), Callback(Callback) {<br>
>  }<br>
><br>
>  bool UnwrappedLineParser::parse() {<br>
> @@ -126,9 +127,9 @@<br>
><br>
>    addUnwrappedLine();<br>
><br>
> -  Line.Level += AddLevels;<br>
> +  Line->Level += AddLevels;<br>
>    parseLevel(/*HasOpeningBrace=*/true);<br>
> -  Line.Level -= AddLevels;<br>
> +  Line->Level -= AddLevels;<br>
><br>
>    if (!<a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::r_brace))<br>
>      return true;<br>
> @@ -139,7 +140,7 @@<br>
><br>
>  void UnwrappedLineParser::parsePPDirective() {<br>
>    assert(<a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::hash) && "'#' expected");<br>
> -  ScopedMacroState MacroState(Line, Tokens, FormatTok);<br>
> +  ScopedMacroState MacroState(*Line, Tokens, FormatTok);<br>
>    nextToken();<br>
><br>
>    if (FormatTok.Tok.getIdentifierInfo() == NULL) {<br>
> @@ -169,7 +170,7 @@<br>
>      parseParens();<br>
>    }<br>
>    addUnwrappedLine();<br>
> -  Line.Level = 1;<br>
> +  Line->Level = 1;<br>
><br>
>    // Errors during a preprocessor directive can only affect the layout of the<br>
>    // preprocessor directive, and thus we ignore them. An alternative approach<br>
> @@ -319,9 +320,9 @@<br>
>      NeedsUnwrappedLine = true;<br>
>    } else {<br>
>      addUnwrappedLine();<br>
> -    ++Line.Level;<br>
> +    ++Line->Level;<br>
>      parseStructuralElement();<br>
> -    --Line.Level;<br>
> +    --Line->Level;<br>
>    }<br>
>    if (<a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::kw_else)) {<br>
>      nextToken();<br>
> @@ -332,9 +333,9 @@<br>
>        parseIfThenElse();<br>
>      } else {<br>
>        addUnwrappedLine();<br>
> -      ++Line.Level;<br>
> +      ++Line->Level;<br>
>        parseStructuralElement();<br>
> -      --Line.Level;<br>
> +      --Line->Level;<br>
>      }<br>
>    } else if (NeedsUnwrappedLine) {<br>
>      addUnwrappedLine();<br>
> @@ -363,9 +364,9 @@<br>
>      addUnwrappedLine();<br>
>    } else {<br>
>      addUnwrappedLine();<br>
> -    ++Line.Level;<br>
> +    ++Line->Level;<br>
>      parseStructuralElement();<br>
> -    --Line.Level;<br>
> +    --Line->Level;<br>
>    }<br>
>  }<br>
><br>
> @@ -376,9 +377,9 @@<br>
>      parseBlock();<br>
>    } else {<br>
>      addUnwrappedLine();<br>
> -    ++Line.Level;<br>
> +    ++Line->Level;<br>
>      parseStructuralElement();<br>
> -    --Line.Level;<br>
> +    --Line->Level;<br>
>    }<br>
><br>
>    // FIXME: Add error handling.<br>
> @@ -395,14 +396,14 @@<br>
>    // FIXME: remove all asserts.<br>
>    assert(<a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::colon) && "':' expected");<br>
>    nextToken();<br>
> -  unsigned OldLineLevel = Line.Level;<br>
> -  if (Line.Level > 0)<br>
> -    --Line.Level;<br>
> +  unsigned OldLineLevel = Line->Level;<br>
> +  if (Line->Level > 0)<br>
> +    --Line->Level;<br>
>    if (<a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::l_brace)) {<br>
>      parseBlock();<br>
>    }<br>
>    addUnwrappedLine();<br>
> -  Line.Level = OldLineLevel;<br>
> +  Line->Level = OldLineLevel;<br>
>  }<br>
><br>
>  void UnwrappedLineParser::parseCaseLabel() {<br>
> @@ -423,9 +424,9 @@<br>
>      addUnwrappedLine();<br>
>    } else {<br>
>      addUnwrappedLine();<br>
> -    Line.Level += (Style.IndentCaseLabels ? 2 : 1);<br>
> +    Line->Level += (Style.IndentCaseLabels ? 2 : 1);<br>
>      parseStructuralElement();<br>
> -    Line.Level -= (Style.IndentCaseLabels ? 2 : 1);<br>
> +    Line->Level -= (Style.IndentCaseLabels ? 2 : 1);<br>
>    }<br>
>  }<br>
><br>
> @@ -444,7 +445,7 @@<br>
>      case tok::l_brace:<br>
>        nextToken();<br>
>        addUnwrappedLine();<br>
> -      ++Line.Level;<br>
> +      ++Line->Level;<br>
>        parseComments();<br>
>        break;<br>
>      case tok::l_paren:<br>
> @@ -458,7 +459,7 @@<br>
>      case tok::r_brace:<br>
>        if (HasContents)<br>
>          addUnwrappedLine();<br>
> -      --Line.Level;<br>
> +      --Line->Level;<br>
>        nextToken();<br>
>        break;<br>
>      case tok::semi:<br>
> @@ -501,8 +502,9 @@<br>
>           <a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::comment)) {<br>
>      nextToken();<br>
>    }<br>
> -  Callback.consumeUnwrappedLine(Line);<br>
> +  Callback.consumeUnwrappedLine(*Line);<br>
>    RootTokenInitialized = false;<br>
> +  LastInCurrentLine = NULL;<br>
>  }<br>
><br>
>  bool UnwrappedLineParser::eof() const {<br>
> @@ -513,26 +515,42 @@<br>
>    if (eof())<br>
>      return;<br>
>    if (RootTokenInitialized) {<br>
> +    assert(LastInCurrentLine->Children.empty());<br>
>      LastInCurrentLine->Children.push_back(FormatTok);<br>
>      LastInCurrentLine = &LastInCurrentLine->Children.back();<br>
>    } else {<br>
> -    Line.RootToken = FormatTok;<br>
> +    Line->RootToken = FormatTok;<br>
>      RootTokenInitialized = true;<br>
> -    LastInCurrentLine = &Line.RootToken;<br>
> +    LastInCurrentLine = &Line->RootToken;<br>
> +  }<br>
> +  if (MustBreakBeforeNextToken) {<br>
> +    LastInCurrentLine->MustBreakBefore = true;<br>
> +    MustBreakBeforeNextToken = false;<br>
>    }<br>
>    readToken();<br>
>  }<br>
><br>
>  void UnwrappedLineParser::readToken() {<br>
>    FormatTok = Tokens->getNextToken();<br>
> -  while (!Line.InPPDirective && <a href="http://FormatTok.Tok.is" target="_blank">FormatTok.Tok.is</a>(tok::hash) &&<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>
> -    // FIXME: This is incorrect - the correct way is to create a<br>
> -    // data structure that will construct the parts around the preprocessor<br>
> -    // directive as a structured \c UnwrappedLine.<br>
> -    addUnwrappedLine();<br>
> +    UnwrappedLine* StoredLine = Line.take();<br>
> +    Line.reset(new UnwrappedLine(*StoredLine));<br>
> +    assert(LastInCurrentLine == NULL || LastInCurrentLine->Children.empty());<br>
> +    FormatToken *StoredLastInCurrentLine = LastInCurrentLine;<br>
> +    bool PreviousInitialized = RootTokenInitialized;<br>
> +    RootTokenInitialized = false;<br>
> +    LastInCurrentLine = NULL;<br>
> +<br>
>      parsePPDirective();<br>
> +<br>
> +    assert(!RootTokenInitialized);<br>
> +    Line.reset(StoredLine);<br>
> +    RootTokenInitialized = PreviousInitialized;<br>
> +    LastInCurrentLine = StoredLastInCurrentLine;<br>
> +    assert(LastInCurrentLine == NULL || LastInCurrentLine->Children.empty());<br>
> +    MustBreakBeforeNextToken = true;<br>
>    }<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=171974&r1=171973&r2=171974&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=171974&r1=171973&r2=171974&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/lib/Format/UnwrappedLineParser.h (original)<br>
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.h Wed Jan  9 09:25:02 2013<br>
> @@ -34,7 +34,7 @@<br>
>  struct FormatToken {<br>
>    FormatToken()<br>
>        : NewlinesBefore(0), HasUnescapedNewline(false), WhiteSpaceLength(0),<br>
> -        TokenLength(0), IsFirst(false) {<br>
> +        TokenLength(0), IsFirst(false), MustBreakBefore(false) {<br>
>    }<br>
><br>
>    /// \brief The \c Token.<br>
> @@ -68,6 +68,12 @@<br>
>    /// \brief Indicates that this is the first token.<br>
>    bool IsFirst;<br>
><br>
> +  /// \brief Whether there must be a line break before this token.<br>
> +  ///<br>
> +  /// This happens for example when a preprocessor directive ended directly<br>
> +  /// before the token.<br>
> +  bool MustBreakBefore;<br>
> +<br>
>    // FIXME: We currently assume that there is exactly one token in this vector<br>
>    // except for the very last token that does not have any children.<br>
>    /// \brief All tokens that logically follow this token.<br>
> @@ -144,10 +150,11 @@<br>
>    // FIXME: We are constantly running into bugs where Line.Level is incorrectly<br>
>    // subtracted from beyond 0. Introduce a method to subtract from Line.Level<br>
>    // and use that everywhere in the Parser.<br>
> -  UnwrappedLine Line;<br>
> +  llvm::OwningPtr<UnwrappedLine> Line;<br>
>    bool RootTokenInitialized;<br>
>    FormatToken *LastInCurrentLine;<br>
>    FormatToken FormatTok;<br>
> +  bool MustBreakBeforeNextToken;<br>
><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=171974&r1=171973&r2=171974&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=171974&r1=171973&r2=171974&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Jan  9 09:25:02 2013<br>
> @@ -46,17 +46,24 @@<br>
>    std::string messUp(llvm::StringRef Code) {<br>
>      std::string MessedUp(Code.str());<br>
>      bool InComment = false;<br>
> +    bool InPreprocessorDirective = false;<br>
>      bool JustReplacedNewline = false;<br>
>      for (unsigned i = 0, e = MessedUp.size() - 1; i != e; ++i) {<br>
>        if (MessedUp[i] == '/' && MessedUp[i + 1] == '/') {<br>
>          if (JustReplacedNewline)<br>
>            MessedUp[i - 1] = '\n';<br>
>          InComment = true;<br>
> +      } else if (MessedUp[i] == '#' && JustReplacedNewline) {<br>
> +        MessedUp[i - 1] = '\n';<br>
> +        InPreprocessorDirective = true;<br>
>        } else if (MessedUp[i] == '\\' && MessedUp[i + 1] == '\n') {<br>
>          MessedUp[i] = ' ';<br>
> +        MessedUp[i + 1] = ' ';<br>
>        } else if (MessedUp[i] == '\n') {<br>
>          if (InComment) {<br>
>            InComment = false;<br>
> +        } else if (InPreprocessorDirective) {<br>
> +          InPreprocessorDirective = false;<br>
>          } else {<br>
>            JustReplacedNewline = true;<br>
>            MessedUp[i] = ' ';<br>
> @@ -84,6 +91,14 @@<br>
>    }<br>
>  };<br>
><br>
> +TEST_F(FormatTest, MessUp) {<br>
> +  EXPECT_EQ("1 2 3", messUp("1 2 3"));<br>
> +  EXPECT_EQ("1 2 3\n", messUp("1\n2\n3\n"));<br>
> +  EXPECT_EQ("a\n//b\nc", messUp("a\n//b\nc"));<br>
> +  EXPECT_EQ("a\n#b\nc", messUp("a\n#b\nc"));<br>
> +  EXPECT_EQ("a\n#b  c  d\ne", messUp("a\n#b\\\nc\\\nd\ne"));<br>
> +}<br>
> +<br>
>  //===----------------------------------------------------------------------===//<br>
>  // Basic function tests.<br>
>  //===----------------------------------------------------------------------===//<br>
> @@ -545,7 +560,9 @@<br>
>  }<br>
><br>
>  TEST_F(FormatTest, MacroDefinitionInsideStatement) {<br>
> -  EXPECT_EQ("int x,\n#define A\ny;", format("int x,\n#define A\ny;"));<br>
> +  EXPECT_EQ("int x,\n"<br>
> +            "#define A\n"<br>
> +            "    y;", format("int x,\n#define A\ny;"));<br>
>  }<br>
><br>
>  TEST_F(FormatTest, HashInMacroDefinition) {<br>
> @@ -609,6 +626,23 @@<br>
>               "  aLooooooooooooooooooooooonPaaaaaaaaaaaaaaaaaaaaarmmmm);\n"));<br>
>  }<br>
><br>
> +TEST_F(FormatTest, LayoutStatementsAroundPreprocessorDirectives) {<br>
> +  EXPECT_EQ("int\n"<br>
> +            "#define A\n"<br>
> +            "    a;",<br>
> +            format("int\n#define A\na;"));<br>
> +  verifyFormat(<br>
> +      "functionCallTo(someOtherFunction(\n"<br>
> +      "    withSomeParameters, whichInSequence,\n"<br>
> +      "    areLongerThanALine(andAnotherCall,\n"<br>
> +      "#define A                                                           \\\n"<br>
> +      "  B\n"<br>
> +      "                       withMoreParamters,\n"<br>
> +      "                       whichStronglyInfluenceTheLayout),\n"<br>
> +      "    andMoreParameters),\n"<br>
> +      "               trailing);", getLLVMStyleWithColumns(69));<br>
> +}<br>
> +<br>
>  //===----------------------------------------------------------------------===//<br>
>  // Line break tests.<br>
>  //===----------------------------------------------------------------------===//<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>