[PATCH] D35955: clang-format: Add preprocessor directive indentation

[Mark Zeren via Phabricator via cfe-commits]

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