euhlmann updated this revision to Diff 110429.
euhlmann edited the summary of this revision.
euhlmann added a comment.
This addresses Daniel's previous concerns. The option is now an enum, the
heuristic for detecting include guards is expanded and has corresponding unit
tests, and style issues are resolved.
This does not yet use `PPBranchLevel` to track indent; I'm updating this review
with my current work before addressing that point.
https://reviews.llvm.org/D35955
Files:
docs/ClangFormatStyleOptions.rst
include/clang/Format/Format.h
lib/Format/ContinuationIndenter.cpp
lib/Format/Format.cpp
lib/Format/UnwrappedLineFormatter.cpp
lib/Format/UnwrappedLineParser.cpp
lib/Format/UnwrappedLineParser.h
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===================================================================
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2286,8 +2286,158 @@
getLLVMStyleWithColumns(11));
}
-TEST_F(FormatTest, IndentPreprocessorDirectivesAtZero) {
- EXPECT_EQ("{\n {\n#define A\n }\n}", format("{{\n#define A\n}}"));
+TEST_F(FormatTest, IndentPreprocessorDirectives) {
+ FormatStyle Style = getLLVMStyle();
+ Style.IndentPPDirectives = FormatStyle::PPDIS_None;
+ verifyFormat("#ifdef _WIN32\n"
+ "#define A 0\n"
+ "#ifdef VAR2\n"
+ "#define B 1\n"
+ "#include <someheader.h>\n"
+ "#define MACRO "
+ " \\\n"
+ " some_very_long_func_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaaa();\n"
+ "#endif\n"
+ "#else\n"
+ "#define A 1\n"
+ "#endif",
+ Style);
+
+ Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+ verifyFormat("#ifdef _WIN32\n"
+ "# define A 0\n"
+ "# ifdef VAR2\n"
+ "# define B 1\n"
+ "# include <someheader.h>\n"
+ "# define MACRO "
+ " \\\n"
+ " some_very_long_func_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaaaaaaa();\n"
+ "# endif\n"
+ "#else\n"
+ "# define A 1\n"
+ "#endif",
+ Style);
+ // 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"
+ "code();\n"
+ "#endif",
+ Style));
+ // Include guards must have a #define with the same variable immediately
+ // after #ifndef.
+ EXPECT_EQ("#ifndef NOT_GUARD\n"
+ "# define FOO\n"
+ "code();\n"
+ "#endif",
+ format("#ifndef NOT_GUARD\n"
+ "# define FOO\n"
+ "code();\n"
+ "#endif",
+ Style));
+
+ // Include guards must cover the entire file.
+ EXPECT_EQ("code();\n"
+ "#ifndef NOT_GUARD\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#endif",
+ format("code();\n"
+ "#ifndef NOT_GUARD\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#endif",
+ Style));
+ EXPECT_EQ("#ifndef NOT_GUARD\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#endif\n"
+ "code();",
+ format("#ifndef NOT_GUARD\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#endif\n"
+ "code();",
+ Style));
+ // Include guards don't have #else.
+ EXPECT_EQ("#ifndef NOT_GUARD\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#else\n"
+ "#endif",
+ format("#ifndef NOT_GUARD\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#else\n"
+ "#endif",
+ Style));
+ EXPECT_EQ("#ifndef NOT_GUARD\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#elif FOO\n"
+ "#endif",
+ format("#ifndef NOT_GUARD\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#elif FOO\n"
+ "#endif",
+ Style));
+ // Defect: We currently do not deal with the case where there's code between
+ // the #ifndef and #define but all other conditions hold. This is because when
+ // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
+ // previous code line yet, so we can't detect it.
+ EXPECT_NE("#ifndef NOT_GUARD\n"
+ "code();\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#endif",
+ format("#ifndef NOT_GUARD\n"
+ "code();\n"
+ "# define NOT_GUARD\n"
+ "code();\n"
+ "#endif",
+ Style));
+ // 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.
+ EXPECT_NE("#ifdef WIN32\n"
+ "# pragma once\n"
+ "#endif\n"
+ "#ifndef _SOMEFILE_H\n"
+ "#define _SOMEFILE_H\n"
+ "code();\n"
+ "#endif",
+ format("#ifdef WIN32\n"
+ "# pragma once\n"
+ "#endif\n"
+ "#ifndef _SOMEFILE_H\n"
+ "#define _SOMEFILE_H\n"
+ "code();\n"
+ "#endif",
+ Style));
+ // Test with tabs.
+ Style.UseTab = FormatStyle::UT_Always;
+ Style.IndentWidth = 8;
+ Style.TabWidth = 8;
+ verifyFormat("#ifdef _WIN32\n"
+ "#\tdefine A 0\n"
+ "#\tifdef VAR2\n"
+ "#\t\tdefine B 1\n"
+ "#\t\tinclude <someheader.h>\n"
+ "#\t\tdefine MACRO \\\n"
+ "\t\t\tsome_very_long_func_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+ "aaaaaaaaaaaaaaaaa();\n"
+ "#\tendif\n"
+ "#else\n"
+ "#\tdefine A 1\n"
+ "#endif",
+ Style);
}
TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) {
Index: lib/Format/UnwrappedLineParser.h
===================================================================
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -246,6 +246,10 @@
// sequence.
std::stack<int> PPChainBranchIndex;
+ unsigned PPIndentLevel;
+ FormatToken *PPMaybeIncludeGuard;
+ bool FoundIncludeGuardStart;
+
friend class ScopedLineState;
friend class CompoundStatementIndenter;
};
Index: lib/Format/UnwrappedLineParser.cpp
===================================================================
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -231,7 +231,9 @@
: Line(new UnwrappedLine), MustBreakBeforeNextToken(false),
CurrentLines(&Lines), Style(Style), Keywords(Keywords),
CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
- Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1) {}
+ Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
+ PPIndentLevel(0), PPMaybeIncludeGuard(nullptr),
+ FoundIncludeGuardStart(false) {}
void UnwrappedLineParser::reset() {
PPBranchLevel = -1;
@@ -691,19 +693,43 @@
if (IfDef && !IfNDef && FormatTok->TokenText == "SWIG")
Unreachable = true;
conditionalCompilationStart(Unreachable);
+ FormatToken *IfCondition = FormatTok;
parsePPUnknown();
+ // If there's a #ifndef on the first line, it could be an include guard.
+ if (IfNDef && Lines.size() == 1)
+ PPMaybeIncludeGuard = IfCondition;
+ ++PPIndentLevel;
}
void UnwrappedLineParser::parsePPElse() {
+ if (PPIndentLevel > 0)
+ --PPIndentLevel;
+ // If a potential include guard has a #else, it's not an include guard.
+ if (FoundIncludeGuardStart && PPIndentLevel == 0)
+ FoundIncludeGuardStart = false;
conditionalCompilationAlternative();
parsePPUnknown();
+ ++PPIndentLevel;
}
void UnwrappedLineParser::parsePPElIf() { parsePPElse(); }
void UnwrappedLineParser::parsePPEndIf() {
+ if (PPIndentLevel > 0)
+ --PPIndentLevel;
conditionalCompilationEnd();
parsePPUnknown();
+ // If the #endif of a potential include guard is the last thing in the file,
+ // then we count it as a real include guard and subtract one from every
+ // preprocessor indent.
+ unsigned TokenPosition = Tokens->getPosition();
+ FormatToken *PeekNext = AllTokens[TokenPosition];
+ if (FoundIncludeGuardStart && PPIndentLevel == 0 && PeekNext->is(tok::eof)) {
+ for (auto it = Lines.begin(); it != Lines.end(); ++it) {
+ if (it->InPPDirective && it->Level > 0)
+ --it->Level;
+ }
+ }
}
void UnwrappedLineParser::parsePPDefine() {
@@ -713,14 +739,22 @@
parsePPUnknown();
return;
}
+ if (PPMaybeIncludeGuard &&
+ PPMaybeIncludeGuard->TokenText == FormatTok->TokenText &&
+ Lines.size() == 1) {
+ FoundIncludeGuardStart = true;
+ }
+ PPMaybeIncludeGuard = nullptr;
nextToken();
if (FormatTok->Tok.getKind() == tok::l_paren &&
FormatTok->WhitespaceRange.getBegin() ==
FormatTok->WhitespaceRange.getEnd()) {
parseParens();
}
+ if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash)
+ Line->Level += PPIndentLevel;
addUnwrappedLine();
- Line->Level = 1;
+ ++Line->Level;
// Errors during a preprocessor directive can only affect the layout of the
// preprocessor directive, and thus we ignore them. An alternative approach
@@ -734,7 +768,10 @@
do {
nextToken();
} while (!eof());
+ if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash)
+ Line->Level += PPIndentLevel;
addUnwrappedLine();
+ PPMaybeIncludeGuard = nullptr;
}
// Here we blacklist certain tokens that are not usually the first token in an
Index: lib/Format/UnwrappedLineFormatter.cpp
===================================================================
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -1037,6 +1037,10 @@
if (RootToken.IsFirst && !RootToken.HasUnescapedNewline)
Newlines = 0;
+ // Preprocessor directives get indented after the hash, if indented.
+ if (Line.Type == LT_PreprocessorDirective || Line.Type == LT_ImportStatement)
+ Indent = 0;
+
// Remove empty lines after "{".
if (!Style.KeepEmptyLinesAtTheStartOfBlocks && PreviousLine &&
PreviousLine->Last->is(tok::l_brace) &&
Index: lib/Format/Format.cpp
===================================================================
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -134,6 +134,14 @@
};
template <>
+struct ScalarEnumerationTraits<FormatStyle::PPDirectiveIndentStyle> {
+ static void enumeration(IO &IO, FormatStyle::PPDirectiveIndentStyle &Value) {
+ IO.enumCase(Value, "None", FormatStyle::PPDIS_None);
+ IO.enumCase(Value, "AfterHash", FormatStyle::PPDIS_AfterHash);
+ }
+};
+
+template <>
struct ScalarEnumerationTraits<FormatStyle::ReturnTypeBreakingStyle> {
static void enumeration(IO &IO, FormatStyle::ReturnTypeBreakingStyle &Value) {
IO.enumCase(Value, "None", FormatStyle::RTBS_None);
@@ -350,6 +358,7 @@
IO.mapOptional("IncludeCategories", Style.IncludeCategories);
IO.mapOptional("IncludeIsMainRegex", Style.IncludeIsMainRegex);
IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
+ IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
IO.mapOptional("IndentWidth", Style.IndentWidth);
IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
@@ -589,6 +598,7 @@
{".*", 1}};
LLVMStyle.IncludeIsMainRegex = "(Test)?$";
LLVMStyle.IndentCaseLabels = false;
+ LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
LLVMStyle.IndentWrappedFunctionNames = false;
LLVMStyle.IndentWidth = 2;
LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
Index: lib/Format/ContinuationIndenter.cpp
===================================================================
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -376,6 +376,19 @@
unsigned Spaces = Current.SpacesRequiredBefore + ExtraSpaces;
+ // Indent preprocessor directives after the hash if required.
+ if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash &&
+ (State.Line->Type == LT_PreprocessorDirective ||
+ State.Line->Type == LT_ImportStatement) &&
+ Current.Previous->is(tok::hash) && State.FirstIndent > 0) {
+ Spaces += State.FirstIndent;
+ // For preprocessor indent with tabs, State.Column will be 1 because of the
+ // hash. This causes second-level indents onward to have an extra space
+ // after the tabs. We set the state to column 0 to avoid this misalignment.
+ if (Style.UseTab != FormatStyle::UT_Never)
+ --State.Column;
+ }
+
if (!DryRun)
Whitespaces.replaceWhitespace(Current, /*Newlines=*/0, Spaces,
State.Column + Spaces);
Index: include/clang/Format/Format.h
===================================================================
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -1024,6 +1024,31 @@
/// \endcode
bool IndentCaseLabels;
+ /// \brief Options for indenting preprocessor directives.
+ enum PPDirectiveIndentStyle {
+ /// Does not indent any directives.
+ /// \code
+ /// #if FOO
+ /// #if BAR
+ /// #include <foo>
+ /// #endif
+ /// #endif
+ /// \endcode
+ PPDIS_None,
+ /// Indents directives after the hash, counting the hash as a column.
+ /// \code
+ /// #if FOO
+ /// # if BAR
+ /// # include <foo>
+ /// # endif
+ /// #endif
+ /// \endcode
+ PPDIS_AfterHash
+ };
+
+ /// \brief The preprocessor directive indenting style to use.
+ PPDirectiveIndentStyle IndentPPDirectives;
+
/// \brief The number of columns to use for indentation.
/// \code
/// IndentWidth: 3
@@ -1514,6 +1539,7 @@
ForEachMacros == R.ForEachMacros &&
IncludeCategories == R.IncludeCategories &&
IndentCaseLabels == R.IndentCaseLabels &&
+ IndentPPDirectives == R.IndentPPDirectives &&
IndentWidth == R.IndentWidth && Language == R.Language &&
IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
JavaScriptQuotes == R.JavaScriptQuotes &&
Index: docs/ClangFormatStyleOptions.rst
===================================================================
--- docs/ClangFormatStyleOptions.rst
+++ docs/ClangFormatStyleOptions.rst
@@ -1179,6 +1179,33 @@
plop(); plop();
} }
+**IndentPPDirectives** (``PPDirectiveIndentStyle``)
+ Indent preprocessor directives on conditionals.
+
+ Possible values:
+
+ * ``PPDIS_None`` (in configuration: ``None``)
+ Does not indent any directives.
+
+ .. code-block:: c++
+
+ #if FOO
+ #if BAR
+ #include <foo>
+ #endif
+ #endif
+
+ * ``PPDIS_AfterHash`` (in configuration: ``AfterHash``)
+ Indents directives after the hash, counting the hash as a column.
+
+ .. code-block:: c++
+
+ #if FOO
+ # if BAR
+ # include <foo>
+ # endif
+ #endif
+
**IndentWidth** (``unsigned``)
The number of columns to use for indentation.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
