[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()
This revision was automatically updated to reflect the committed changes.
Closed by commit rL324238: [clang-format] Fixup #include guard indents after
parseFile() (authored by mzeren-vmw, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D42035?vs=132394=132819#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42035
Files:
cfe/trunk/lib/Format/UnwrappedLineParser.cpp
cfe/trunk/lib/Format/UnwrappedLineParser.h
cfe/trunk/unittests/Format/FormatTest.cpp
Index: cfe/trunk/lib/Format/UnwrappedLineParser.h
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.h
+++ cfe/trunk/lib/Format/UnwrappedLineParser.h
@@ -248,10 +248,23 @@
// sequence.
std::stack PPChainBranchIndex;
- // Contains the #ifndef condition for a potential include guard.
- FormatToken *IfNdefCondition;
- bool FoundIncludeGuardStart;
- bool IncludeGuardRejected;
+ // Include guard search state. Used to fixup preprocessor indent levels
+ // so that include guards do not participate in indentation.
+ enum IncludeGuardState {
+IG_Inited,
+IG_IfNdefed,
+IG_Defined,
+IG_Found,
+IG_Rejected,
+ };
+
+ // Current state of include guard search.
+ IncludeGuardState IncludeGuard;
+
+ // Points to the #ifndef condition for a potential include guard. Null unless
+ // IncludeGuardState == IG_IfNdefed.
+ FormatToken *IncludeGuardToken;
+
// Contains the first start column where the source begins. This is zero for
// normal source code and may be nonzero when formatting a code fragment that
// does not start at the beginning of the file.
Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -234,14 +234,15 @@
CurrentLines(), Style(Style), Keywords(Keywords),
CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
- IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
- IncludeGuardRejected(false), FirstStartColumn(FirstStartColumn) {}
+ IncludeGuard(Style.IndentPPDirectives == FormatStyle::PPDIS_None
+ ? IG_Rejected
+ : IG_Inited),
+ IncludeGuardToken(nullptr), FirstStartColumn(FirstStartColumn) {}
void UnwrappedLineParser::reset() {
PPBranchLevel = -1;
- IfNdefCondition = nullptr;
- FoundIncludeGuardStart = false;
- IncludeGuardRejected = false;
+ IncludeGuard = IG_Inited;
+ IncludeGuardToken = nullptr;
Line.reset(new UnwrappedLine);
CommentsBeforeNextToken.clear();
FormatTok = nullptr;
@@ -264,6 +265,14 @@
readToken();
parseFile();
+
+// If we found an include guard then all preprocessor directives (other than
+// the guard) are over-indented by one.
+if (IncludeGuard == IG_Found)
+ for (auto : Lines)
+if (Line.InPPDirective && Line.Level > 0)
+ --Line.Level;
+
// Create line with eof token.
pushToken(FormatTok);
addUnwrappedLine();
@@ -724,26 +733,28 @@
// 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) {
+ if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
for (auto : Lines) {
if (!Line.Tokens.front().Tok->is(tok::comment)) {
MaybeIncludeGuard = false;
-IncludeGuardRejected = true;
+IncludeGuard = IG_Rejected;
break;
}
}
}
--PPBranchLevel;
parsePPUnknown();
++PPBranchLevel;
- if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard)
-IfNdefCondition = IfCondition;
+ if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
+IncludeGuard = IG_IfNdefed;
+IncludeGuardToken = IfCondition;
+ }
}
void UnwrappedLineParser::parsePPElse() {
// If a potential include guard has an #else, it's not an include guard.
- if (FoundIncludeGuardStart && PPBranchLevel == 0)
-FoundIncludeGuardStart = false;
+ if (IncludeGuard == IG_Defined && PPBranchLevel == 0)
+IncludeGuard = IG_Rejected;
conditionalCompilationAlternative();
if (PPBranchLevel > -1)
--PPBranchLevel;
@@ -757,34 +768,37 @@
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.
+ // then we found an include guard.
unsigned TokenPosition = Tokens->getPosition();
FormatToken *PeekNext = AllTokens[TokenPosition];
- if (FoundIncludeGuardStart && PPBranchLevel == -1 && PeekNext->is(tok::eof) &&
+ if (IncludeGuard == IG_Defined && PPBranchLevel == -1
[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()
krasimir accepted this revision.
krasimir added a comment.
This revision is now accepted and ready to land.
Looks good! Thank you!
Comment at: lib/Format/UnwrappedLineParser.cpp:244
PPBranchLevel = -1;
- IfNdefCondition = nullptr;
- FoundIncludeGuardStart = false;
- IncludeGuardRejected = false;
+ IncludeGuard = IG_Inited;
+ IncludeGuardToken = nullptr;
mzeren-vmw wrote:
> Hm. From self review, I think this should be:
>
> IncludeGuard = Style.IndentPPDirectives == FormatStyle::PPDIS_None ?
> IG_Rejected : IG_Inited;
Makes sense.
Comment at: lib/Format/UnwrappedLineParser.cpp:736
bool MaybeIncludeGuard = IfNDef;
- if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard) {
+ if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
for (auto : Lines) {
mzeren-vmw wrote:
> technically I could drop the braces opened on this line. Would you like me to
> do that?
I like the braces.
Repository:
rC Clang
https://reviews.llvm.org/D42035
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()
mzeren-vmw updated this revision to Diff 132394.
mzeren-vmw added a comment.
- Add comments to IncludeGuardState.
- Fix re-initialization of IncludeGuard in UnwrappedLineParser::reset.
- Remove unnecessary block after if.
- Rebase
Repository:
rC Clang
https://reviews.llvm.org/D42035
Files:
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
@@ -2547,6 +2547,20 @@
"#elif FOO\n"
"#endif",
Style);
+ // Non-identifier #define after potential include guard.
+ verifyFormat("#ifndef FOO\n"
+ "# define 1\n"
+ "#endif\n",
+ Style);
+ // #if closes past last non-preprocessor line.
+ verifyFormat("#ifndef FOO\n"
+ "#define FOO\n"
+ "#if 1\n"
+ "int i;\n"
+ "# define A 0\n"
+ "#endif\n"
+ "#endif\n",
+ 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
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -248,10 +248,23 @@
// sequence.
std::stack PPChainBranchIndex;
- // Contains the #ifndef condition for a potential include guard.
- FormatToken *IfNdefCondition;
- bool FoundIncludeGuardStart;
- bool IncludeGuardRejected;
+ // Include guard search state. Used to fixup preprocessor indent levels
+ // so that include guards do not participate in indentation.
+ enum IncludeGuardState {
+IG_Inited, // Search started, looking for #ifndef.
+IG_IfNdefed, // #ifndef found, IncludeGuardToken points to condition.
+IG_Defined, // Matching #define found, checking other requirements.
+IG_Found,// All requirements met, need to fix indents.
+IG_Rejected, // Search failed or never started.
+ };
+
+ // Current state of include guard search.
+ IncludeGuardState IncludeGuard;
+
+ // Points to the #ifndef condition for a potential include guard. Null unless
+ // IncludeGuardState == IG_IfNdefed.
+ FormatToken *IncludeGuardToken;
+
// Contains the first start column where the source begins. This is zero for
// normal source code and may be nonzero when formatting a code fragment that
// does not start at the beginning of the file.
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -234,14 +234,17 @@
CurrentLines(), Style(Style), Keywords(Keywords),
CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
- IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
- IncludeGuardRejected(false), FirstStartColumn(FirstStartColumn) {}
+ IncludeGuard(Style.IndentPPDirectives == FormatStyle::PPDIS_None
+ ? IG_Rejected
+ : IG_Inited),
+ IncludeGuardToken(nullptr), FirstStartColumn(FirstStartColumn) {}
void UnwrappedLineParser::reset() {
PPBranchLevel = -1;
- IfNdefCondition = nullptr;
- FoundIncludeGuardStart = false;
- IncludeGuardRejected = false;
+ IncludeGuard = Style.IndentPPDirectives == FormatStyle::PPDIS_None
+ ? IG_Rejected
+ : IG_Inited;
+ IncludeGuardToken = nullptr;
Line.reset(new UnwrappedLine);
CommentsBeforeNextToken.clear();
FormatTok = nullptr;
@@ -264,6 +267,14 @@
readToken();
parseFile();
+
+// If we found an include guard then all preprocessor directives (other than
+// the guard) are over-indented by one.
+if (IncludeGuard == IG_Found)
+ for (auto : Lines)
+if (Line.InPPDirective && Line.Level > 0)
+ --Line.Level;
+
// Create line with eof token.
pushToken(FormatTok);
addUnwrappedLine();
@@ -724,26 +735,27 @@
// 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) {
+ if (IncludeGuard == IG_Inited && MaybeIncludeGuard)
for (auto : Lines) {
if (!Line.Tokens.front().Tok->is(tok::comment)) {
MaybeIncludeGuard = false;
-IncludeGuardRejected = true;
+IncludeGuard = IG_Rejected;
break;
}
}
- }
--PPBranchLevel;
parsePPUnknown();
++PPBranchLevel;
- if (!IncludeGuardRejected &&
[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()
mzeren-vmw added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:244
PPBranchLevel = -1;
- IfNdefCondition = nullptr;
- FoundIncludeGuardStart = false;
- IncludeGuardRejected = false;
+ IncludeGuard = IG_Inited;
+ IncludeGuardToken = nullptr;
Hm. From self review, I think this should be:
IncludeGuard = Style.IndentPPDirectives == FormatStyle::PPDIS_None ?
IG_Rejected : IG_Inited;
Comment at: lib/Format/UnwrappedLineParser.cpp:736
bool MaybeIncludeGuard = IfNDef;
- if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard) {
+ if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
for (auto : Lines) {
technically I could drop the braces opened on this line. Would you like me to
do that?
Repository:
rC Clang
https://reviews.llvm.org/D42035
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()
krasimir added a comment. Looks good! Just some comments for the include guard states would be helpful. Comment at: lib/Format/UnwrappedLineParser.h:259 +IG_Rejected, + }; + Please put a short comment explaining each of these states. Repository: rC Clang https://reviews.llvm.org/D42035 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()
mzeren-vmw updated this revision to Diff 132320.
mzeren-vmw added a comment.
rebase, ping.
Repository:
rC Clang
https://reviews.llvm.org/D42035
Files:
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
@@ -2548,6 +2548,20 @@
"#elif FOO\n"
"#endif",
Style);
+ // Non-identifier #define after potential include guard.
+ verifyFormat("#ifndef FOO\n"
+ "# define 1\n"
+ "#endif\n",
+ Style);
+ // #if closes past last non-preprocessor line.
+ verifyFormat("#ifndef FOO\n"
+ "#define FOO\n"
+ "#if 1\n"
+ "int i;\n"
+ "# define A 0\n"
+ "#endif\n"
+ "#endif\n",
+ 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
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -248,10 +248,23 @@
// sequence.
std::stack PPChainBranchIndex;
- // Contains the #ifndef condition for a potential include guard.
- FormatToken *IfNdefCondition;
- bool FoundIncludeGuardStart;
- bool IncludeGuardRejected;
+ // Include guard search state. Used to fixup preprocessor indent levels
+ // so that include guards do not participate in indentation.
+ enum IncludeGuardState {
+IG_Inited,
+IG_IfNdefed,
+IG_Defined,
+IG_Found,
+IG_Rejected,
+ };
+
+ // Current state of include guard search.
+ IncludeGuardState IncludeGuard;
+
+ // Points to the #ifndef condition for a potential include guard. Null unless
+ // IncludeGuardState == IG_IfNdefed.
+ FormatToken *IncludeGuardToken;
+
// Contains the first start column where the source begins. This is zero for
// normal source code and may be nonzero when formatting a code fragment that
// does not start at the beginning of the file.
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -234,14 +234,15 @@
CurrentLines(), Style(Style), Keywords(Keywords),
CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
- IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
- IncludeGuardRejected(false), FirstStartColumn(FirstStartColumn) {}
+ IncludeGuard(Style.IndentPPDirectives == FormatStyle::PPDIS_None
+ ? IG_Rejected
+ : IG_Inited),
+ IncludeGuardToken(nullptr), FirstStartColumn(FirstStartColumn) {}
void UnwrappedLineParser::reset() {
PPBranchLevel = -1;
- IfNdefCondition = nullptr;
- FoundIncludeGuardStart = false;
- IncludeGuardRejected = false;
+ IncludeGuard = IG_Inited;
+ IncludeGuardToken = nullptr;
Line.reset(new UnwrappedLine);
CommentsBeforeNextToken.clear();
FormatTok = nullptr;
@@ -264,6 +265,14 @@
readToken();
parseFile();
+
+// If we found an include guard then all preprocessor directives (other than
+// the guard) are over-indented by one.
+if (IncludeGuard == IG_Found)
+ for (auto : Lines)
+if (Line.InPPDirective && Line.Level > 0)
+ --Line.Level;
+
// Create line with eof token.
pushToken(FormatTok);
addUnwrappedLine();
@@ -724,26 +733,28 @@
// 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) {
+ if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
for (auto : Lines) {
if (!Line.Tokens.front().Tok->is(tok::comment)) {
MaybeIncludeGuard = false;
-IncludeGuardRejected = true;
+IncludeGuard = IG_Rejected;
break;
}
}
}
--PPBranchLevel;
parsePPUnknown();
++PPBranchLevel;
- if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard)
-IfNdefCondition = IfCondition;
+ if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
+IncludeGuard = IG_IfNdefed;
+IncludeGuardToken = IfCondition;
+ }
}
void UnwrappedLineParser::parsePPElse() {
// If a potential include guard has an #else, it's not an include guard.
- if (FoundIncludeGuardStart && PPBranchLevel == 0)
-FoundIncludeGuardStart = false;
+ if (IncludeGuard == IG_Defined && PPBranchLevel == 0)
+
[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()
mzeren-vmw created this revision.
mzeren-vmw added reviewers: euhlmann, krasimir, klimek.
Herald added a subscriber: cfe-commits.
When a preprocessor indent closes after the last line of normal code we do not
correctly fixup include guard indents. For example:
#ifndef HEADER_H
#define HEADER_H
#if 1
int i;
# define A 0
#endif
#endif
incorrectly reformats to:
#ifndef HEADER_H
#define HEADER_H
#if 1
int i;
#define A 0
# endif
#endif
To resolve this issue we must fixup levels after parseFile(). Delaying
the fixup introduces a new state, so consolidate include guard search
state into an enum.
Repository:
rC Clang
https://reviews.llvm.org/D42035
Files:
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
@@ -2531,6 +2531,20 @@
"#elif FOO\n"
"#endif",
Style);
+ // Non-identifier #define after potential include guard.
+ verifyFormat("#ifndef FOO\n"
+ "# define 1\n"
+ "#endif\n",
+ Style);
+ // #if closes past last non-preprocessor line.
+ verifyFormat("#ifndef FOO\n"
+ "#define FOO\n"
+ "#if 1\n"
+ "int i;\n"
+ "# define A 0\n"
+ "#endif\n"
+ "#endif\n",
+ 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
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -248,10 +248,23 @@
// sequence.
std::stack PPChainBranchIndex;
- // Contains the #ifndef condition for a potential include guard.
- FormatToken *IfNdefCondition;
- bool FoundIncludeGuardStart;
- bool IncludeGuardRejected;
+ // Include guard search state. Used to fixup preprocessor indent levels
+ // so that include guards do not participate in indentation.
+ enum IncludeGuardState {
+IG_Inited,
+IG_IfNdefed,
+IG_Defined,
+IG_Found,
+IG_Rejected,
+ };
+
+ // Current state of include guard search.
+ IncludeGuardState IncludeGuard;
+
+ // Points to the #ifndef condition for a potential include guard. Null unless
+ // IncludeGuardState == IG_IfNdefed.
+ FormatToken *IncludeGuardToken;
+
// Contains the first start column where the source begins. This is zero for
// normal source code and may be nonzero when formatting a code fragment that
// does not start at the beginning of the file.
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -234,14 +234,15 @@
CurrentLines(), Style(Style), Keywords(Keywords),
CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
- IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
- IncludeGuardRejected(false), FirstStartColumn(FirstStartColumn) {}
+ IncludeGuard(Style.IndentPPDirectives == FormatStyle::PPDIS_None
+ ? IG_Rejected
+ : IG_Inited),
+ IncludeGuardToken(nullptr), FirstStartColumn(FirstStartColumn) {}
void UnwrappedLineParser::reset() {
PPBranchLevel = -1;
- IfNdefCondition = nullptr;
- FoundIncludeGuardStart = false;
- IncludeGuardRejected = false;
+ IncludeGuard = IG_Inited;
+ IncludeGuardToken = nullptr;
Line.reset(new UnwrappedLine);
CommentsBeforeNextToken.clear();
FormatTok = nullptr;
@@ -264,6 +265,14 @@
readToken();
parseFile();
+
+// If we found an include guard then all preprocessor directives (other than
+// the guard) are over-indented by one.
+if (IncludeGuard == IG_Found)
+ for (auto : Lines)
+if (Line.InPPDirective && Line.Level > 0)
+ --Line.Level;
+
// Create line with eof token.
pushToken(FormatTok);
addUnwrappedLine();
@@ -712,26 +721,28 @@
// 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) {
+ if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
for (auto : Lines) {
if (!Line.Tokens.front().Tok->is(tok::comment)) {
MaybeIncludeGuard = false;
-IncludeGuardRejected = true;
+IncludeGuard = IG_Rejected;
break;
}
}
}
--PPBranchLevel;
parsePPUnknown();
