djasper added inline comments.
================
Comment at: unittests/Format/FormatTest.cpp:2281
TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) {
verifyFormat("#define A \\\n"
----------------
mzeren-vmw wrote:
> mzeren-vmw wrote:
> > Experimenting with the patch locally I found that comment indentation is
> > broken in some cases. Please add tests that cover comments. For example:
> >
> > comment indented at code level as expected:
> > void f() {
> > #if 0
> > // Comment
> > code();
> > # define FOO 0
> > #endif
> > }
> > comment not indented at code level when there's a guard:
> > #ifndef _SOMEFILE_H
> > #define _SOMEFILE_H
> > void f() {
> > #if 0
> > // Comment
> > code();
> > # define FOO 0
> > #endif
> > }
> > #endif
> >
> > The `#define FOO 0` is required to trigger this behavior.
> Erik spent some time investigating issues with comment indentation. A couple
> of insights so far:
>
> a) We need tests for wrapped macros (with trailing \ continuations). These
> need to include embedded comments.
>
> b) It would be most natural to align comments with the following non-comment
> line. So a comment may be indented at "preprocessor level" or "code level".
> If indented at preprocessor level we have to handle the extra space
> introduced by the leading hash. This may require an extra bit per line to
> indicate whether the "aligned-to" line is preprocessor or code.
"#if 0" specifically might not be a good candidate for a test case as that
should be treated like a comment. We should not try to format anything inside a
"#if 0" and I'd be surprised if clang-format touched that at all. "#if A" or
something is more appropriate.
================
Comment at: unittests/Format/FormatTest.cpp:2322-2331
+ // Test with include guards.
+ EXPECT_EQ("#ifndef _SOMEFILE_H\n"
+ "#define _SOMEFILE_H\n"
+ "code();\n"
+ "#endif",
+ format("#ifndef _SOMEFILE_H\n"
+ "#define _SOMEFILE_H\n"
----------------
mzeren-vmw wrote:
> A DRY-er way to say this is:
> auto WithGuard = "#ifndef _SOMEFILE_H\n"
> "#define _SOMEFILE_H\n"
> "code();\n"
> "#endif";
> ASSERT_EQ(WithGuards, format(WithGuards, Style));
>
I think _A... is actually not an identifier that is ok to use. Remove the
leading "_".
================
Comment at: unittests/Format/FormatTest.cpp:2334
+ // after #ifndef.
+ EXPECT_EQ("#ifndef NOT_GUARD\n"
+ "# define FOO\n"
----------------
mzeren-vmw wrote:
>
> It would be nice to have a comment that explains why you cannot use
> verifyFormat.
Or even have a separate function that messes this up in some other way, not
altering line breaks so that we can keep these tests short.
================
Comment at: unittests/Format/FormatTest.cpp:2405-2408
+ // Defect: We currently do not deal with cases where legitimate lines may be
+ // outside an include guard. Examples are #pragma once and
+ // #pragma GCC diagnostic, or anything else that does not change the meaning
+ // of the file if it's included multiple times.
----------------
euhlmann wrote:
> mzeren-vmw wrote:
> > We need to handle comments (like copyrights) before the include guard.
> > There is an analogous problem with trailing blank lines, or trailing
> > comments.
> >
> > I think we need a small state machine: Init, Start, Open, Closed, NotGuard
> > (terminal). `FoundIncludeGuardStart` should change from a bool to an enum
> > to track this state. `PPMaybeIncludeGuard` can then drop it's "mabye".
> > Whether or not it is null depends directly on the state. It does not
> > determine state itself. While parsing, each line updates the state. If we
> > get to `eof` in the Closed state then we have detected an include guard.
> > ... Or similar logic....
> >
> > Note that support for comments before the guard opens the door to support
> > for #pragma once. It is tempting to add that, but that is a slippery slope.
> > I would recommend leaving that as a defect that can be addressed later.
> @djasper @klimek
> Do you have any comments on this? I've begun to implement an enum/state
> machine but I'd like to know if you'd like me to implement a different way.
Most of the work here is done in the UnwrappedLineParser, which should skip
over comments anyway. So yes, we should allow for these comments, but I don't
think the logic needs to get much more complex because of that. I strongly
doubt that a state machine is the right mechanism here.
And generally (without specific action to derive from that), I think we should
try to err on the side of incorrectly finding header guards instead of
incorrectly not recognizing a header guard. I think a not-indented #define
somewhere is better than an incorrectly indented header guard.
https://reviews.llvm.org/D35955
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
