<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 16, 2014 at 7:41 PM, Benjamin Kramer <span dir="ltr"><<a href="mailto:benny.kra@googlemail.com" target="_blank">benny.kra@googlemail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: d0k<br>
Date: Tue Sep 16 12:41:19 2014<br>
New Revision: 217890<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=217890&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=217890&view=rev</a><br>
Log:<br>
[clang-tidy] When emitting header guard fixes bundle all fix-its into one<br>
warning.<br>
<br>
Before we would emit two warnings if the header guard was wrong and the comment<br>
on a trailing #endif also needed fixing.<br>
<br>
Modified:<br>
clang-tools-extra/trunk/clang-tidy/utils/HeaderGuard.cpp<br>
clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp<br>
<br>
Modified: clang-tools-extra/trunk/clang-tidy/utils/HeaderGuard.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/HeaderGuard.cpp?rev=217890&r1=217889&r2=217890&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/HeaderGuard.cpp?rev=217890&r1=217889&r2=217890&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/utils/HeaderGuard.cpp (original)<br>
+++ clang-tools-extra/trunk/clang-tidy/utils/HeaderGuard.cpp Tue Sep 16 12:41:19 2014<br>
@@ -109,8 +109,7 @@ public:<br>
EndIfs[Ifndefs[MacroEntry.first.getIdentifierInfo()].first];<br>
<br>
// If the macro Name is not equal to what we can compute, correct it in<br>
- // the<br>
- // #ifndef and #define.<br>
+ // the #ifndef and #define.<br>
StringRef CurHeaderGuard =<br>
MacroEntry.first.getIdentifierInfo()->getName();<br>
std::string NewGuard = checkHeaderGuardDefinition(<br>
@@ -119,6 +118,22 @@ public:<br>
// Now look at the #endif. We want a comment with the header guard. Fix it<br>
// at the slightest deviation.<br>
checkEndifComment(FileName, EndIf, NewGuard);<br>
+<br>
+ // Bundle all fix-its into one warning. The message depends on whether we<br>
+ // changed the header guard or not.<br>
+ if (!FixIts.empty()) {<br>
+ if (CurHeaderGuard != NewGuard) {<br>
+ auto D = Check->diag(Ifndef,<br>
+ "header guard does not follow preferred style");<br>
+ for (const FixItHint Fix : FixIts)<br>
+ D.AddFixItHint(Fix);<br>
+ } else {<br>
+ auto D = Check->diag(EndIf, "#endif for a header guard should "<br>
+ "reference the guard macro in a comment");<br>
+ for (const FixItHint Fix : FixIts)<br>
+ D.AddFixItHint(Fix);<br>
+ }<br>
+ }<br>
}<br>
<br>
// Emit warnings for headers that are missing guards.<br>
@@ -129,6 +144,7 @@ public:<br>
Files.clear();<br>
Ifndefs.clear();<br>
EndIfs.clear();<br>
+ FixIts.clear();<br>
}<br>
<br>
bool wouldFixEndifComment(StringRef FileName, SourceLocation EndIf,<br>
@@ -170,15 +186,14 @@ public:<br>
if (Ifndef.isValid() && CurHeaderGuard != CPPVar &&<br>
(CurHeaderGuard != CPPVarUnder ||<br>
wouldFixEndifComment(FileName, EndIf, CurHeaderGuard))) {<br>
- Check->diag(Ifndef, "header guard does not follow preferred style")<br>
- << FixItHint::CreateReplacement(<br>
- CharSourceRange::getTokenRange(<br>
- Ifndef, Ifndef.getLocWithOffset(CurHeaderGuard.size())),<br>
- CPPVar)<br>
- << FixItHint::CreateReplacement(<br>
- CharSourceRange::getTokenRange(<br>
- Define, Define.getLocWithOffset(CurHeaderGuard.size())),<br>
- CPPVar);<br>
+ FixIts.push_back(FixItHint::CreateReplacement(<br>
+ CharSourceRange::getTokenRange(<br>
+ Ifndef, Ifndef.getLocWithOffset(CurHeaderGuard.size())),<br>
+ CPPVar));<br>
+ FixIts.push_back(FixItHint::CreateReplacement(<br>
+ CharSourceRange::getTokenRange(<br>
+ Define, Define.getLocWithOffset(CurHeaderGuard.size())),<br>
+ CPPVar));<br>
return CPPVar;<br>
}<br>
return CurHeaderGuard;<br>
@@ -191,12 +206,10 @@ public:<br>
size_t EndIfLen;<br>
if (wouldFixEndifComment(FileName, EndIf, HeaderGuard, &EndIfLen)) {<br>
std::string Correct = "endif // " + HeaderGuard.str();<br>
- Check->diag(EndIf, "#endif for a header guard should reference the "<br>
- "guard macro in a comment")<br>
- << FixItHint::CreateReplacement(<br>
- CharSourceRange::getCharRange(EndIf,<br>
- EndIf.getLocWithOffset(EndIfLen)),<br>
- Correct);<br>
+ FixIts.push_back(FixItHint::CreateReplacement(<br>
+ CharSourceRange::getCharRange(EndIf,<br>
+ EndIf.getLocWithOffset(EndIfLen)),<br>
+ Correct));<br>
}<br>
}<br>
<br>
@@ -257,6 +270,7 @@ private:<br>
std::map<const IdentifierInfo *, std::pair<SourceLocation, SourceLocation>><br>
Ifndefs;<br>
std::map<SourceLocation, SourceLocation> EndIfs;<br>
+ std::vector<FixItHint> FixIts;<br>
<br>
Preprocessor *PP;<br>
HeaderGuardCheck *Check;<br>
<br>
Modified: clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp?rev=217890&r1=217889&r2=217890&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp?rev=217890&r1=217889&r2=217890&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp (original)<br>
+++ clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp Tue Sep 16 12:41:19 2014<br>
@@ -88,9 +88,12 @@ TEST(NamespaceCommentCheckTest, FixWrong<br>
<br>
// FIXME: It seems this might be incompatible to dos path. Investigating.<br>
#if !defined(_WIN32)<br>
-static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename) {<br>
- return test::runCheckOnCode<LLVMHeaderGuardCheck>(<br>
- Code, /*Errors=*/nullptr, Filename, std::string("-xc++-header"));<br>
+static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename,<br>
+ unsigned NumWarnings = 1) {<br>
+ std::vector<ClangTidyError> Errors;<br>
+ std::string Result = test::runCheckOnCode<LLVMHeaderGuardCheck>(<br>
+ Code, &Errors, Filename, std::string("-xc++-header"));<br>
+ return Errors.size() == NumWarnings ? Result : "invalid error count";<br>
}<br>
<br>
namespace {<br>
@@ -102,9 +105,12 @@ struct WithEndifComment : public LLVMHea<br>
} // namespace<br>
<br>
static std::string runHeaderGuardCheckWithEndif(StringRef Code,<br>
- const Twine &Filename) {<br>
- return test::runCheckOnCode<WithEndifComment>(<br>
- Code, /*Errors=*/nullptr, Filename, std::string("-xc++-header"));<br>
+ const Twine &Filename,<br>
+ unsigned NumWarnings = 1) {<br></blockquote><div><br></div><div>Not sure if you've got <a href="http://reviews.llvm.org/rL217890#inline-284">this comment</a>. Repeating it here:</div><div>I'd remove the default value here and added argument comments (/*NumWarnings=*/) for this parameter to the tests to make the expectations more explicit.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ std::vector<ClangTidyError> Errors;<br>
+ std::string Result = test::runCheckOnCode<WithEndifComment>(<br>
+ Code, &Errors, Filename, std::string("-xc++-header"));<br>
+ return Errors.size() == NumWarnings ? Result : "invalid error count";<br>
}<br>
<br>
TEST(LLVMHeaderGuardCheckTest, FixHeaderGuards) {<br>
@@ -116,7 +122,7 @@ TEST(LLVMHeaderGuardCheckTest, FixHeader<br>
EXPECT_EQ("#ifndef LLVM_ADT_FOO_H_\n#define LLVM_ADT_FOO_H_\n#endif\n",<br>
runHeaderGuardCheck(<br>
"#ifndef LLVM_ADT_FOO_H_\n#define LLVM_ADT_FOO_H_\n#endif\n",<br>
- "include/llvm/ADT/foo.h"));<br>
+ "include/llvm/ADT/foo.h", 0));<br>
<br>
EXPECT_EQ("#ifndef LLVM_CLANG_C_BAR_H\n#define LLVM_CLANG_C_BAR_H\n\n\n#endif\n",<br>
runHeaderGuardCheck("", "./include/clang-c/bar.h"));<br>
@@ -157,14 +163,14 @@ TEST(LLVMHeaderGuardCheckTest, FixHeader<br>
runHeaderGuardCheckWithEndif("#ifndef LLVM_ADT_FOO_H\n#define "<br>
"LLVM_ADT_FOO_H\n"<br>
"#endif /* LLVM_ADT_FOO_H */\n",<br>
- "include/llvm/ADT/foo.h"));<br>
+ "include/llvm/ADT/foo.h", 0));<br>
<br>
EXPECT_EQ("#ifndef LLVM_ADT_FOO_H_\n#define LLVM_ADT_FOO_H_\n#endif "<br>
"// LLVM_ADT_FOO_H_\n",<br>
runHeaderGuardCheckWithEndif(<br>
"#ifndef LLVM_ADT_FOO_H_\n#define "<br>
"LLVM_ADT_FOO_H_\n#endif // LLVM_ADT_FOO_H_\n",<br>
- "include/llvm/ADT/foo.h"));<br>
+ "include/llvm/ADT/foo.h", 0));<br>
<br>
EXPECT_EQ(<br>
"#ifndef LLVM_ADT_FOO_H\n#define LLVM_ADT_FOO_H\n#endif // "<br>
@@ -178,14 +184,14 @@ TEST(LLVMHeaderGuardCheckTest, FixHeader<br>
runHeaderGuardCheckWithEndif("#ifndef LLVM_ADT_FOO_H\n#define "<br>
"LLVM_ADT_FOO_H\n#endif \\ \n// "<br>
"LLVM_ADT_FOO_H\n",<br>
- "include/llvm/ADT/foo.h"));<br>
+ "include/llvm/ADT/foo.h", 0));<br>
<br>
EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n#define LLVM_ADT_FOO_H\n#endif /* "<br>
"LLVM_ADT_FOO_H\\ \n FOO */",<br>
runHeaderGuardCheckWithEndif(<br>
"#ifndef LLVM_ADT_FOO_H\n#define LLVM_ADT_FOO_H\n#endif /* "<br>
"LLVM_ADT_FOO_H\\ \n FOO */",<br>
- "include/llvm/ADT/foo.h"));<br>
+ "include/llvm/ADT/foo.h", 0));<br>
}<br>
#endif<br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><div><div><font color="#666666"><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(213,15,37);padding-top:2px;margin-top:2px">Alexander Kornienko |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(51,105,232);padding-top:2px;margin-top:2px"> Software Engineer |</span></font><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(0,153,57);padding-top:2px;margin-top:2px"><font color="#666666"> </font><a href="mailto:alexfh@google.com" style="color:rgb(17,85,204)" target="_blank">alexfh@google.com</a> |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(238,178,17);padding-top:2px;margin-top:2px"> Google Germany, Munich</span></div></div></div>
</div></div>