<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 8, 2014 at 3:10 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Wed, Oct 8, 2014 at 1:45 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.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"><br><br><div class="gmail_quote"><div><div>On Wed Oct 08 2014 at 11:34:02 AM Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>> 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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Oct 7, 2014 at 7:49 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.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: klimek<br>
Date: Tue Oct 7 10:49:36 2014<br>
New Revision: 219212<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=219212&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=219212&view=rev</a><br>
Log:<br>
Make clang-tidy's runCheckOnCode actually use the DiagnosticConsumer.<br>
<br>
A precondition of that was to run both the preprocessor checks and AST<br>
checks from the same FrontendAction, otherwise we'd have needed to<br>
duplicate all involved objects in order to not have any references to a<br>
deleted source manager.<br>
<br>
Modified:<br>
clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h<br>
clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp<br>
<br>
Modified: clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h?rev=219212&r1=219211&r2=219212&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h?rev=219212&r1=219211&r2=219212&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h (original)<br>
+++ clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h Tue Oct 7 10:49:36 2014<br>
@@ -22,21 +22,23 @@ namespace clang {<br>
namespace tidy {<br>
namespace test {<br>
<br>
-class TestPPAction : public PreprocessOnlyAction {<br>
+class TestClangTidyAction : public ASTFrontendAction {<br>
public:<br>
- TestPPAction(ClangTidyCheck &Check, ClangTidyContext *Context)<br>
- : Check(Check), Context(Context) {}<br>
+ TestClangTidyAction(ClangTidyCheck &Check, ast_matchers::MatchFinder &Finder,<br>
+ ClangTidyContext &Context)<br>
+ : Check(Check), Finder(Finder), Context(Context) {}<br>
<br>
private:<br>
- bool BeginSourceFileAction(CompilerInstance &Compiler,<br>
- llvm::StringRef file_name) override {<br>
- Context->setSourceManager(&Compiler.getSourceManager());<br>
+ std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,<br>
+ StringRef File) override {<br>
+ Context.setSourceManager(&Compiler.getSourceManager());<br>
Check.registerPPCallbacks(Compiler);<br>
- return true;<br>
+ return Finder.newASTConsumer();<br>
}<br>
<br>
ClangTidyCheck &Check;<br>
- ClangTidyContext *Context;<br>
+ ast_matchers::MatchFinder &Finder;<br>
+ ClangTidyContext &Context;<br>
};<br>
<br>
template <typename T><br>
@@ -50,19 +52,25 @@ std::string runCheckOnCode(StringRef Cod<br>
ClangTidyGlobalOptions(), Options));<br>
ClangTidyDiagnosticConsumer DiagConsumer(Context);<br>
T Check("test-check", &Context);<br>
- std::vector<std::string> ArgCXX11(1, "-std=c++11");<br>
- ArgCXX11.insert(ArgCXX11.end(), ExtraArgs.begin(), ExtraArgs.end());<br>
-<br>
- if (!tooling::runToolOnCodeWithArgs(new TestPPAction(Check, &Context), Code,<br>
- ArgCXX11, Filename))<br>
- return "";<br>
ast_matchers::MatchFinder Finder;<br>
Check.registerMatchers(&Finder);<br>
- std::unique_ptr<tooling::FrontendActionFactory> Factory(<br>
- tooling::newFrontendActionFactory(&Finder));<br>
- if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, ArgCXX11,<br>
- Filename))<br>
+<br>
+ std::vector<std::string> ArgCXX11(1, "clang-tidy");<br>
+ ArgCXX11.push_back("-fsyntax-only");<br>
+ ArgCXX11.push_back("-std=c++11");<br>
+ ArgCXX11.insert(ArgCXX11.end(), ExtraArgs.begin(), ExtraArgs.end());<br>
+ ArgCXX11.push_back(Filename.str());<br>
+ llvm::IntrusiveRefCntPtr<FileManager> files(<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Please capitalize "files". </div></div></div></div></blockquote><div><br></div></div></div><div>Willdo.</div><span><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Also, why do we use llvm::IntrusiveRefCntPtr rather than std::unique_ptr? (I guess there's a reason, but I don't see it.)</div></div></div></div></blockquote><div><br></div></span><div>This is the same code as in Tooling. I'd like to share that again at some point, but didn't see a good way to resolve this now - if you have ideas, I'm all ears.</div></div></blockquote><div><br></div></div></div><div>Answering my own question: it can't be unique_ptr, because "invoking "delete" ... on such [derived from RefCountedBase<>] objects is an error."</div></div></div></div></blockquote><div><br></div><div>I don't think that's the case - I believe you can singly/externally own a RefCountedBase<> object. Just don't mix unique_ptr or direct value instances with IntrusiveRefCntPtr ownership to the same entity.<br><br>And that's the problem here - go a few layers down (ToolInvocation::ToolInvocation -> ToolInvocation::runInvocation -> FrontendActionFactory::runInvocation -> CompilerInstance::setFileManager), and CompilerInstance intrusively takes shared ownership of this object. Now, granted, the CompilerInstance will be destroyed when that function returns, so its ownership is strictly dominated by the ownership of the 'files' variable in Manuel's code... <br><br>But this is one of those ownership puzzles I keep coming across (one flavor of it anyway) where it might be nice for a caller to say "no, it's OK, I've got this, you don't need to take ownership". (& in several APIs we have this kind of conditional ownership - take ToolInvocations handling of the Action+OwnsAction flag, it'd be possible for CompilerInstance to do a similar thing - but I'm not sure it's the right answer (I've been trying to have a decent discussion about this on llvm-dev... but maybe I need to bring it to cfe-dev too, since I see this idiom more in Clang than LLVM))<br><br>Alternatively, of course - push the ref counting explicitly through the APIs and at least move to std::shared_ptr without intrusion if possible (shared_ptr supports intrusion if necessary) even though in this case it's not necessary.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><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"><div class="gmail_quote"><span><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
+ new FileManager(FileSystemOptions()));<br>
+ tooling::ToolInvocation Invocation(<br>
+ ArgCXX11, new TestClangTidyAction(Check, Finder, Context), files.get());<br>
+ SmallString<16> FileNameStorage;<br>
+ StringRef FileNameRef = Filename.toNullTerminatedStringRef(FileNameStorage); </blockquote><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">
+ Invocation.mapVirtualFile(FileNameRef, Code);<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Maybe Invocation.mapVirtualFile(Filename.str(), Code)?</div></div></div></div></blockquote><div><br></div></span><div>Yea, you're right - since this is for testing, we can always copy. Again, I think the right way is to make the interface with tooling simpler to use.</div></div></blockquote><div><br></div></span><div>Copying of a filename doesn't have many chances of becoming a bottleneck ;)</div><div><div class="h5"><div><br></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"><div class="gmail_quote"><span><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
+ Invocation.setDiagnosticConsumer(&DiagConsumer);<br>
+ if (!Invocation.run())<br>
return "";<br>
+<br>
DiagConsumer.finish();<br>
tooling::Replacements Fixes;<br>
for (const ClangTidyError &Error : Context.getErrors())<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=219212&r1=219211&r2=219212&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/LLVMModuleTest.cpp?rev=219212&r1=219211&r2=219212&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 Oct 7 10:49:36 2014<br>
@@ -114,7 +114,7 @@ TEST(LLVMHeaderGuardCheckTest, FixHeader<br>
"LLVM_ADT_FOO_H\n#endif \\ \n// "<br>
"LLVM_ADT_FOO_H\n",<br>
"include/llvm/ADT/foo.h",<br>
- /*ExpectedWarnings=*/0));<br>
+ /*ExpectedWarnings=*/1));<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>What kind of warning started appearing here? About unknown escape sequence?</div></div></div></div></blockquote><div><br></div></span><div>Space between \ and EOL. I'm still confused why this warning didn't trigger go through, but the others did...</div><span><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>"ExpectedWarnings" doesn't help much =\ I'll change this to actual warning messages some day.</div></div></div></div></blockquote><div><br></div></span><div>Oh yes - I had to add print statements to figure out what's going on at all - which was a rather annoying experience ;)</div><span><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
<br>
EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n#define LLVM_ADT_FOO_H\n#endif /* "<br>
"LLVM_ADT_FOO_H\\ \n FOO */",<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">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></div></div></blockquote></span></div>
</blockquote></div></div></div><br><br>
</div></div>
<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>
<br></blockquote></div><br></div></div>