Author: krasimir Date: Wed Aug 30 07:34:57 2017 New Revision: 312125 URL: http://llvm.org/viewvc/llvm-project?rev=312125&view=rev Log: clang-format: Add preprocessor directive indentation
Summary: This is an implementation for [bug 17362](https://bugs.llvm.org/attachment.cgi?bugid=17362) which adds support for indenting preprocessor statements inside if/ifdef/endif. This takes previous work from fmauch (https://github.com/fmauch/clang/tree/preprocessor_indent) and makes it into a full feature. The context of this patch is that I'm a VMware intern, and I implemented this because VMware needs the feature. As such, some decisions were made based on what VMware wants, and I would appreciate suggestions on expanding this if necessary to use-cases other people may want. This adds a new enum config option, `IndentPPDirectives`. Values are: * `PPDIS_None` (in config: `None`): ``` #if FOO #if BAR #include <foo> #endif #endif ``` * `PPDIS_AfterHash` (in config: `AfterHash`): ``` #if FOO # if BAR # include <foo> # endif #endif ``` This is meant to work whether spaces or tabs are used for indentation. Preprocessor indentation is independent of indentation for non-preprocessor lines. Preprocessor indentation also attempts to ignore include guards with the checks: 1. Include guards cover the entire file 2. Include guards don't have `#else` 3. Include guards begin with ``` #ifndef <var> #define <var> ``` This patch allows `UnwrappedLineParser::PPBranchLevel` to be decremented to -1 (the initial value is -1) so the variable can be used for indent tracking. Defects: * This patch does not handle 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. This is out of the scope of this patch. * This patch does not handle 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. * This does not detect when there is a single non-preprocessor line in front of an include-guard-like structure where other conditions hold because `ScopedLineState` hides the line. * Preprocessor indentation throws off `TokenAnnotator::setCommentLineLevels` so the indentation of comments immediately before indented preprocessor lines is toggled on each run. Fixing this issue appears to be a major change and too much complexity for this patch. Contributed by @euhlmann! Reviewers: djasper, klimek, krasimir Reviewed By: djasper, krasimir Subscribers: krasimir, mzeren-vmw, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D35955 Modified: cfe/trunk/docs/ClangFormatStyleOptions.rst cfe/trunk/include/clang/Format/Format.h cfe/trunk/lib/Format/ContinuationIndenter.cpp cfe/trunk/lib/Format/Format.cpp cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp cfe/trunk/lib/Format/UnwrappedLineParser.cpp cfe/trunk/lib/Format/UnwrappedLineParser.h cfe/trunk/unittests/Format/FormatTest.cpp cfe/trunk/unittests/Format/FormatTestUtils.h Modified: cfe/trunk/docs/ClangFormatStyleOptions.rst URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ClangFormatStyleOptions.rst?rev=312125&r1=312124&r2=312125&view=diff ============================================================================== --- cfe/trunk/docs/ClangFormatStyleOptions.rst (original) +++ cfe/trunk/docs/ClangFormatStyleOptions.rst Wed Aug 30 07:34:57 2017 @@ -1179,6 +1179,33 @@ the configuration (without a prefix: ``A 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. + + .. code-block:: c++ + + #if FOO + # if BAR + # include <foo> + # endif + #endif + **IndentWidth** (``unsigned``) The number of columns to use for indentation. Modified: cfe/trunk/include/clang/Format/Format.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Format/Format.h?rev=312125&r1=312124&r2=312125&view=diff ============================================================================== --- cfe/trunk/include/clang/Format/Format.h (original) +++ cfe/trunk/include/clang/Format/Format.h Wed Aug 30 07:34:57 2017 @@ -1024,6 +1024,31 @@ struct FormatStyle { /// \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. + /// \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 @@ struct FormatStyle { ForEachMacros == R.ForEachMacros && IncludeCategories == R.IncludeCategories && IndentCaseLabels == R.IndentCaseLabels && + IndentPPDirectives == R.IndentPPDirectives && IndentWidth == R.IndentWidth && Language == R.Language && IndentWrappedFunctionNames == R.IndentWrappedFunctionNames && JavaScriptQuotes == R.JavaScriptQuotes && Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=312125&r1=312124&r2=312125&view=diff ============================================================================== --- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original) +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Wed Aug 30 07:34:57 2017 @@ -93,6 +93,13 @@ LineState ContinuationIndenter::getIniti LineState State; State.FirstIndent = FirstIndent; State.Column = FirstIndent; + // With preprocessor directive indentation, the line starts on column 0 + // since it's indented after the hash, but FirstIndent is set to the + // preprocessor indent. + if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash && + (Line->Type == LT_PreprocessorDirective || + Line->Type == LT_ImportStatement)) + State.Column = 0; State.Line = Line; State.NextToken = Line->First; State.Stack.push_back(ParenState(FirstIndent, FirstIndent, @@ -376,9 +383,25 @@ void ContinuationIndenter::addTokenOnCur unsigned Spaces = Current.SpacesRequiredBefore + ExtraSpaces; + // Indent preprocessor directives after the hash if required. + int PPColumnCorrection = 0; + if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash && + Previous.is(tok::hash) && State.FirstIndent > 0 && + (State.Line->Type == LT_PreprocessorDirective || + State.Line->Type == LT_ImportStatement)) { + 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 avoid this misalignment by subtracting 1 from the + // column value passed to replaceWhitespace(). + if (Style.UseTab != FormatStyle::UT_Never) + PPColumnCorrection = -1; + } + if (!DryRun) Whitespaces.replaceWhitespace(Current, /*Newlines=*/0, Spaces, - State.Column + Spaces); + State.Column + Spaces + PPColumnCorrection); // If "BreakBeforeInheritanceComma" mode, don't break within the inheritance // declaration unless there is multiple inheritance. Modified: cfe/trunk/lib/Format/Format.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=312125&r1=312124&r2=312125&view=diff ============================================================================== --- cfe/trunk/lib/Format/Format.cpp (original) +++ cfe/trunk/lib/Format/Format.cpp Wed Aug 30 07:34:57 2017 @@ -134,6 +134,14 @@ template <> struct ScalarEnumerationTrai }; 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 @@ template <> struct MappingTraits<FormatS 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 @@ FormatStyle getLLVMStyle() { {".*", 1}}; LLVMStyle.IncludeIsMainRegex = "(Test)?$"; LLVMStyle.IndentCaseLabels = false; + LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None; LLVMStyle.IndentWrappedFunctionNames = false; LLVMStyle.IndentWidth = 2; LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave; Modified: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp?rev=312125&r1=312124&r2=312125&view=diff ============================================================================== --- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp (original) +++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp Wed Aug 30 07:34:57 2017 @@ -1037,6 +1037,10 @@ void UnwrappedLineFormatter::formatFirst 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) && Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=312125&r1=312124&r2=312125&view=diff ============================================================================== --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original) +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Wed Aug 30 07:34:57 2017 @@ -231,10 +231,15 @@ UnwrappedLineParser::UnwrappedLineParser : 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), + IfNdefCondition(nullptr), FoundIncludeGuardStart(false), + IncludeGuardRejected(false) {} void UnwrappedLineParser::reset() { PPBranchLevel = -1; + IfNdefCondition = nullptr; + FoundIncludeGuardStart = false; + IncludeGuardRejected = false; Line.reset(new UnwrappedLine); CommentsBeforeNextToken.clear(); FormatTok = nullptr; @@ -679,7 +684,7 @@ void UnwrappedLineParser::conditionalCom } } // Guard against #endif's without #if. - if (PPBranchLevel > 0) + if (PPBranchLevel > -1) --PPBranchLevel; if (!PPChainBranchIndex.empty()) PPChainBranchIndex.pop(); @@ -696,12 +701,35 @@ void UnwrappedLineParser::parsePPIf(bool if (IfDef && !IfNDef && FormatTok->TokenText == "SWIG") Unreachable = true; conditionalCompilationStart(Unreachable); + FormatToken *IfCondition = FormatTok; + // If there's a #ifndef on the first line, and the only lines before it are + // comments, it could be an include guard. + bool MaybeIncludeGuard = IfNDef; + if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard) { + for (auto &Line : Lines) { + if (!Line.Tokens.front().Tok->is(tok::comment)) { + MaybeIncludeGuard = false; + IncludeGuardRejected = true; + break; + } + } + } + --PPBranchLevel; parsePPUnknown(); + ++PPBranchLevel; + if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard) + IfNdefCondition = IfCondition; } void UnwrappedLineParser::parsePPElse() { + // If a potential include guard has an #else, it's not an include guard. + if (FoundIncludeGuardStart && PPBranchLevel == 0) + FoundIncludeGuardStart = false; conditionalCompilationAlternative(); + if (PPBranchLevel > -1) + --PPBranchLevel; parsePPUnknown(); + ++PPBranchLevel; } void UnwrappedLineParser::parsePPElIf() { parsePPElse(); } @@ -709,6 +737,17 @@ void UnwrappedLineParser::parsePPElIf() void UnwrappedLineParser::parsePPEndIf() { 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 && PPBranchLevel == -1 && PeekNext->is(tok::eof)) { + for (auto &Line : Lines) { + if (Line.InPPDirective && Line.Level > 0) + --Line.Level; + } + } } void UnwrappedLineParser::parsePPDefine() { @@ -718,14 +757,26 @@ void UnwrappedLineParser::parsePPDefine( parsePPUnknown(); return; } + if (IfNdefCondition && IfNdefCondition->TokenText == FormatTok->TokenText) { + FoundIncludeGuardStart = true; + for (auto &Line : Lines) { + if (!Line.Tokens.front().Tok->isOneOf(tok::comment, tok::hash)) { + FoundIncludeGuardStart = false; + break; + } + } + } + IfNdefCondition = nullptr; nextToken(); if (FormatTok->Tok.getKind() == tok::l_paren && FormatTok->WhitespaceRange.getBegin() == FormatTok->WhitespaceRange.getEnd()) { parseParens(); } + if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash) + Line->Level += PPBranchLevel + 1; 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 @@ -739,7 +790,10 @@ void UnwrappedLineParser::parsePPUnknown do { nextToken(); } while (!eof()); + if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash) + Line->Level += PPBranchLevel + 1; addUnwrappedLine(); + IfNdefCondition = nullptr; } // Here we blacklist certain tokens that are not usually the first token in an Modified: cfe/trunk/lib/Format/UnwrappedLineParser.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=312125&r1=312124&r2=312125&view=diff ============================================================================== --- cfe/trunk/lib/Format/UnwrappedLineParser.h (original) +++ cfe/trunk/lib/Format/UnwrappedLineParser.h Wed Aug 30 07:34:57 2017 @@ -246,6 +246,11 @@ private: // sequence. std::stack<int> PPChainBranchIndex; + // Contains the #ifndef condition for a potential include guard. + FormatToken *IfNdefCondition; + bool FoundIncludeGuardStart; + bool IncludeGuardRejected; + friend class ScopedLineState; friend class CompoundStatementIndenter; }; Modified: cfe/trunk/unittests/Format/FormatTest.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=312125&r1=312124&r2=312125&view=diff ============================================================================== --- cfe/trunk/unittests/Format/FormatTest.cpp (original) +++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Aug 30 07:34:57 2017 @@ -2312,8 +2312,183 @@ TEST_F(FormatTest, LayoutMacroDefinition 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; + Style.ColumnLimit = 40; + 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_aaaaaaaaaa();\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_aaaaaaaaaa();\n" + "# endif\n" + "#else\n" + "# define A 1\n" + "#endif", + Style); + verifyFormat("#if A\n" + "# define MACRO \\\n" + " void a(int x) { \\\n" + " b(); \\\n" + " c(); \\\n" + " d(); \\\n" + " e(); \\\n" + " f(); \\\n" + " }\n" + "#endif", + Style); + // Comments before include guard. + verifyFormat("// file comment\n" + "// file comment\n" + "#ifndef HEADER_H\n" + "#define HEADER_H\n" + "code();\n" + "#endif", + Style); + // Test with include guards. + // EXPECT_EQ is used because verifyFormat() calls messUp() which incorrectly + // merges lines. + verifyFormat("#ifndef HEADER_H\n" + "#define HEADER_H\n" + "code();\n" + "#endif", + Style); + // Include guards must have a #define with the same variable immediately + // after #ifndef. + verifyFormat("#ifndef NOT_GUARD\n" + "# define FOO\n" + "code();\n" + "#endif", + Style); + + // Include guards must cover the entire file. + verifyFormat("code();\n" + "code();\n" + "#ifndef NOT_GUARD\n" + "# define NOT_GUARD\n" + "code();\n" + "#endif", + Style); + verifyFormat("#ifndef NOT_GUARD\n" + "# define NOT_GUARD\n" + "code();\n" + "#endif\n" + "code();", + Style); + // Test with trailing blank lines. + verifyFormat("#ifndef HEADER_H\n" + "#define HEADER_H\n" + "code();\n" + "#endif\n", + Style); + // Include guards don't have #else. + verifyFormat("#ifndef NOT_GUARD\n" + "# define NOT_GUARD\n" + "code();\n" + "#else\n" + "#endif", + Style); + verifyFormat("#ifndef NOT_GUARD\n" + "# define NOT_GUARD\n" + "code();\n" + "#elif FOO\n" + "#endif", + Style); + // FIXME: This doesn't handle 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_EQ("#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)); + // FIXME: This doesn't handle cases where legitimate preprocessor 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_EQ("#ifdef WIN32\n" + "# pragma once\n" + "#endif\n" + "#ifndef HEADER_H\n" + "# define HEADER_H\n" + "code();\n" + "#endif", + format("#ifdef WIN32\n" + "# pragma once\n" + "#endif\n" + "#ifndef HEADER_H\n" + "#define HEADER_H\n" + "code();\n" + "#endif", + Style)); + // FIXME: This does not detect when there is a single non-preprocessor line + // in front of an include-guard-like structure where other conditions hold + // because ScopedLineState hides the line. + EXPECT_EQ("code();\n" + "#ifndef HEADER_H\n" + "#define HEADER_H\n" + "code();\n" + "#endif", + format("code();\n" + "#ifndef HEADER_H\n" + "# define HEADER_H\n" + "code();\n" + "#endif", + Style)); + // FIXME: The comment indent corrector in TokenAnnotator gets thrown off by + // preprocessor indentation. + EXPECT_EQ("#if 1\n" + " // comment\n" + "# define A 0\n" + "// comment\n" + "# define B 0\n" + "#endif", + format("#if 1\n" + "// comment\n" + "# define A 0\n" + " // comment\n" + "# define B 0\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_aaaaaaaaaa();\n" + "#\tendif\n" + "#else\n" + "#\tdefine A 1\n" + "#endif", + Style); } TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) { Modified: cfe/trunk/unittests/Format/FormatTestUtils.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestUtils.h?rev=312125&r1=312124&r2=312125&view=diff ============================================================================== --- cfe/trunk/unittests/Format/FormatTestUtils.h (original) +++ cfe/trunk/unittests/Format/FormatTestUtils.h Wed Aug 30 07:34:57 2017 @@ -30,7 +30,8 @@ inline std::string messUp(llvm::StringRe if (JustReplacedNewline) MessedUp[i - 1] = '\n'; InComment = true; - } else if (MessedUp[i] == '#' && (JustReplacedNewline || i == 0)) { + } else if (MessedUp[i] == '#' && + (JustReplacedNewline || i == 0 || MessedUp[i - 1] == '\n')) { if (i != 0) MessedUp[i - 1] = '\n'; InPreprocessorDirective = true; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits