r178261 - Fixed handling of comments before preprocessor directives.

[Manuel Klimek]

On Tue, Apr 2, 2013 at 12:04 AM, Alexander Kornienko <alexfh at google.com>wrote:

>
> On Apr 1, 2013 6:21 PM, "Manuel Klimek" <klimek at google.com> wrote:
> >
> > On Thu, Mar 28, 2013 at 7:40 PM, Alexander Kornienko <alexfh at google.com>
> wrote:
> >>
> >> Author: alexfh
> >> Date: Thu Mar 28 13:40:55 2013
> >> New Revision: 178261
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=178261&view=rev
> >> Log:
> >> Fixed handling of comments before preprocessor directives.
> >>
> >> Comments before preprocessor directives used to be stored with
> InPPDirective
> >> flag set, which prevented correct comment splitting in this case. Fixed
> by
> >> flushing comments before switching on InPPDirective. Added a new test
> and fixed
> >> one of the existing tests.
> >>
> >>
> >> Modified:
> >>     cfe/trunk/lib/Format/UnwrappedLineParser.cpp
> >>     cfe/trunk/unittests/Format/FormatTest.cpp
> >>
> >> Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
> >> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=178261&r1=178260&r2=178261&view=diff
> >>
> ==============================================================================
> >> --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)
> >> +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Thu Mar 28 13:40:55
> 2013
> >> @@ -822,6 +822,7 @@ void UnwrappedLineParser::readToken() {
> >>      while (!Line->InPPDirective && FormatTok.Tok.is(tok::hash) &&
> >>             ((FormatTok.NewlinesBefore > 0 &&
> FormatTok.HasUnescapedNewline) ||
> >>              FormatTok.IsFirst)) {
> >
> >
> > Please add a comment indicating why we need this.
>
> By "this" you mean the flushComments call I suppose? I wanted to add a
> comment, but "flush comments before starting to parse the preprocessor
> directive" seems like rephrasing the code. Or you can suggest a better
> comment?
>

In general, when I ask for a comment, I ask for an answer to the question
"why are we doing it", not "what are we doing" (I'm pretty good at seeing
what the code does :P)

Since the patch seems to have problems (Daniel just discovered some
regressions) we can talk later about what exactly to do here.


> >
> >>
> >> +      flushComments(FormatTok.NewlinesBefore > 0);
> >>        // If there is an unfinished unwrapped line, we flush the
> preprocessor
> >>        // directives only after that unwrapped line was finished later.
> >>        bool SwitchToPreprocessorLines =
> >>
> >> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
> >> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=178261&r1=178260&r2=178261&view=diff
> >>
> ==============================================================================
> >> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
> >> +++ cfe/trunk/unittests/Format/FormatTest.cpp Thu Mar 28 13:40:55 2013
> >> @@ -699,6 +699,13 @@ TEST_F(FormatTest, SplitsLongCxxComments
> >>        "//Don't add leading\n"
> >>        "//whitespace",
> >>        format("//Don't add leading whitespace",
> getLLVMStyleWithColumns(20)));
> >> +  EXPECT_EQ("// A comment before\n"
> >> +            "// a macro\n"
> >> +            "// definition\n"
> >> +            "#define a b",
> >> +            format("// A comment before a macro definition\n"
> >> +                   "#define a b",
> >> +                   getLLVMStyleWithColumns(20)));
> >>  }
> >>
> >>  TEST_F(FormatTest, SplitsLongLinesInComments) {
> >> @@ -1203,13 +1210,13 @@ TEST_F(FormatTest, IndentsPPDirectiveInR
> >>  }
> >>
> >>  TEST_F(FormatTest, HandlePreprocessorDirectiveContext) {
> >> -  EXPECT_EQ("// some comment\n"
> >> +  EXPECT_EQ("// somecomment\n"
> >
> >
> > What's the reason for this part of the change?
>
> This is the smallest change possible to make this test pass after we
> started breaking long line comments. There are many alternative solutions,
> of course.
>

Ah, I missed the column restriction. Makes sense then.


> >
> >>
> >>              "#include \"a.h\"\n"
> >>              "#define A(  \\\n"
> >>              "    A, B)\n"
> >>              "#include \"b.h\"\n"
> >>              "// somecomment\n",
> >> -            format("  // some comment\n"
> >> +            format("  // somecomment\n"
> >>                     "  #include \"a.h\"\n"
> >>                     "#define A(A,\\\n"
> >>                     "    B)\n"
> >>
> >>
> >> _______________________________________________
> >> 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/20130402/9ba33ecd/attachment.html>