mzeren-vmw added a comment.
Daniel's right. We need need substantially more tests!
================
Comment at: docs/ClangFormatStyleOptions.rst:1199
+ * ``PPDIS_AfterHash`` (in configuration: ``AfterHash``)
+ Indents directives after the hash, counting the hash as a column.
+
----------------
delete outdated bit of description: ", counting the hash as a column"
================
Comment at: include/clang/Format/Format.h:1038
+ PPDIS_None,
+ /// Indents directives after the hash, counting the hash as a column.
+ /// \code
----------------
ditto
================
Comment at: lib/Format/UnwrappedLineParser.cpp:707
void UnwrappedLineParser::parsePPElse() {
+ // If a potential include guard has a #else, it's not an include guard.
+ if (FoundIncludeGuardStart && PPBranchLevel == 0)
----------------
... has an #else ...
================
Comment at: lib/Format/UnwrappedLineParser.cpp:728
+ if (FoundIncludeGuardStart && PPBranchLevel == -1 && PeekNext->is(tok::eof))
{
+ for (auto it = Lines.begin(); it != Lines.end(); ++it) {
+ if (it->InPPDirective && it->Level > 0)
----------------
You can use a range based for loop here. Also locals begin with upper case:
for (auto& Line : Lines) {
================
Comment at: lib/Format/UnwrappedLineParser.cpp:742-744
+ if (PPMaybeIncludeGuard &&
+ PPMaybeIncludeGuard->TokenText == FormatTok->TokenText &&
+ Lines.size() == 1) {
----------------
Lines.size() == 1 is problematic. We must at least support comments before the
include guard. I'll comment on the tests and point out a couple of important
cases that currently fail.
================
Comment at: unittests/Format/FormatTest.cpp:2281
TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) {
verifyFormat("#define A \\\n"
----------------
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.
================
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"
----------------
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));
================
Comment at: unittests/Format/FormatTest.cpp:2334
+ // after #ifndef.
+ EXPECT_EQ("#ifndef NOT_GUARD\n"
+ "# define FOO\n"
----------------
It would be nice to have a comment that explains why you cannot use
verifyFormat.
================
Comment at: unittests/Format/FormatTest.cpp:2394
+ // previous code line yet, so we can't detect it.
+ EXPECT_NE("#ifndef NOT_GUARD\n"
+ "code();\n"
----------------
The two "Defect:" cases would be more precise and more self-documenting if they
used EXPECT_EQ where the "expected" text showed the incorrect results.
================
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.
----------------
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.
https://reviews.llvm.org/D35955
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
