<br><br><div class="gmail_quote">On Wed Oct 08 2014 at 7:54:25 PM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> 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">On Wed, Oct 8, 2014 at 10:36 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><br><div class="gmail_quote"><div><div>On Wed Oct 08 2014 at 6:57:22 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> 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">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>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></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></div></div></div></blockquote><div><br></div></div></div><div>I personally think that if there's a chance that a user of the class wants to keep ownership, the class should not take ownership.</div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I'm sort of inclined to agree, though I haven't quite tried to fight that fight on any API updates I've made so far - feel free to pitch in once I add the ownership discussion to cfe-dev. </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>I remember that ToolInvocation ended up that way because I tried to solve another puzzle related to ownership down the stack.</div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Hmm? The ToolInvocation one looks actually fairly simple. One ctor doesn't take ownership (because the user provides a pointer to a ToolAction that the user will own) the other takes owenrship (because you provide a FrontendAction that will be returned from the SingleFrontendActionFactory that's created internally - thus it must be owned internally).<br><br>This is the semi-common idiom for conditional ownership, though in some cases in reverse: a default value or a user-provided value. (eg: in some cases it's a "well, either I build a stream, or I use stdin - if I built it I need to own it, if I use stdin I don't")</div></div></div></div></blockquote><div><br></div><div>Usually I would argue strongly for "make the user own and inject whatever you need". I'm not sure any more what drove that exact decision back in the day, but I'm not one to make that lightly (I think it's a pain), so I probably thought this will make the using code significantly simpler.</div><div><br></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"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><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"><div>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></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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><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><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" 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>
<br></blockquote></div></div></div></blockquote></div></div></div>
</blockquote></div></div></div></blockquote></div>