<div dir="ltr">Shuai - have you had a chance to look at this?</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Oct 22, 2018 at 4:43 PM Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<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_quote"><div dir="ltr">On Mon, 22 Oct 2018 at 14:57, David Blaikie via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Richard - any further thoughts here (re: layering/dependencies, etc)? Would love to get the layering oddity fixed rather than having it get more embedded over time.</div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>Here's the intended current directory layout and layering as I understand it:</div><div><br></div><div> * include/clang/Analysis and lib/Analysis are the parts of the static analysis engine that are depended on by both Sema and StaticAnalyzer, and include common functionality / infrastructure</div><div> * include/clang/Analysis/Analyses and lib/Analysis/Analyses are the specific Sema analyses (that don't depend on the static analyzer); StaticAnalyzer should not need to depend on this</div><div> * the StaticAnalyzer library should contain all the pieces that are specific to the static analyzer</div><div> * Sema should not depend on Tooling, but it's fine for StaticAnalyzer to depend on Tooling<br></div><div> </div><div>Compared to where we are today, there are two differences:</div><div><br></div><div>1) The implementations of the headers in include/clang/Analysis are in lib/Analysis, not in lib/Analysis/Analyses</div><div>2) ExprMutationAnalyzer is in lib/Analysis despite not being part of the common infrastructure depended on by Sema and StaticAnalyzer</div><div><br></div><div>For point 1, we should change the lib/Analysis directory layout to match the headers.</div><div>For point 2, we should find a home for this ExprMutationAnalyzer code that makes sense. Given that the intent is to use it from the static analyzer, that seems like a reasonable home for it, but libTooling would also make some sense (perhaps a new subdirectory Tooling/Analysis), since it's only performing AST matching, not a flow-sensitive / path-sensitive static analysis.</div></div></div><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div dir="ltr">On Tue, Oct 2, 2018 at 2:44 PM Shuai Wang via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<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_quote"><div dir="ltr">On Mon, Oct 1, 2018 at 4:58 PM Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<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_quote"><div dir="ltr">On Mon, 1 Oct 2018 at 16:10, George Karpenkov via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space">Hi Richard,<br><div><br><blockquote type="cite"><div>On Oct 1, 2018, at 2:50 PM, Richard Smith via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:</div><br class="m_-2648424425002921941m_2510836037432491171m_7276093403816344403m_-8659058378591544385m_2966259377005578923m_6761069030435347266Apple-interchange-newline"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>This looks like the wrong fix to me, but I don't really know enough about what's being done with ExprMutationAnalyzer to have an opinion on what the right fix is.</div><div><br></div><div>Shuai, what is the goal here? Why is this code being moved to Analysis/?</div></div></div></div></div></div></div></blockquote><div><br></div><div>I’ve asked for this possibility, as I wanted to use it from the Clang static analyzer.</div><br><blockquote type="cite"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div> Do you intend to call it from the compiler frontend at some point? I can see a code review for the move itself, but no discussion of a plan or overall direction being followed here. Did I miss it?</div><div><br></div><div>We have historically decided to not use the tooling interfaces (ASTMatcher, ParentMap, etc) from the frontend,</div></div></div></div></div></div></div></blockquote><div><br></div><div>Clang analyzer uses ASTMatcher all over the place.</div><br><blockquote type="cite"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div> because they're generally a poor fit for our goals (we aim for a largely-single-pass compilation with good locality, and the AST matchers make different design choices)</div></div></div></div></div></div></div></blockquote><div><br></div><div>That’s totally good for the analyzer though, right?</div></div></div></blockquote><div><br></div><div>Yes, that all seems fine for the static analyzer.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space"><div><div>In any case, in future it might make sense to move the analyzer out of Clang proper.</div><div>But for know the only way to use clang-tidy utilities from the analyzer is to move them into Clang.</div></div></div></blockquote><div><br></div><div><div>Right. I think we should try to maintain the idea that all the Clang Static Analyzer-specific things are in lib/StaticAnalyzer, and lib/Analysis only contains things depended on by the frontend. (That is: the things a clang binary would use if we didn't link in the static analyzer)</div><div><br></div><div>Given the above, I think the best approach would be to move this code out of lib/Analysis and into somewhere in lib/StaticAnalyzer. There shouldn't be any problem with clang-tidy using it from there, since it already depends on the static analyzer.</div></div></div></div></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div>I'm happy to do the move.</div><div>Could you (or someone) help point out where exactly I should move it to though? </div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space"><div><blockquote type="cite"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>. If you want to change that, we'll need to discuss that decision.</div><div><br></div><div>If the idea is to move this code into clang proper so that it can be used by various different tooling clients, we'd need to discuss the right place for it. Perhaps lib/Tooling/Analysis would make sense?</div></div></div></div></div></div></div></blockquote><blockquote type="cite"><div><br><div class="gmail_quote"><div dir="ltr">On Mon, 1 Oct 2018 at 13:13, David Blaikie via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I can't really reproduce this - when I try to build clang/llvm with LLVM_ENABLE_MODULES in CMake I'm still seeing an error I reported March on a cfe-dev thread - something to do with unique_ptr instantiations for MappedBlockStream in the PDB parsing code.<br><br>So, I'm wondering what error you hit, Eric/where/how, etc... <br><br><div class="gmail_quote"><div dir="ltr">On Sun, Sep 30, 2018 at 10:23 AM Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">+rsmith (actually this time)<br></div><br><div class="gmail_quote"><div dir="ltr">On Sun, Sep 30, 2018 at 12:09 PM Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">+rsmith<br><div><br></div><div>Hi All,</div><div><br></div><div>Sorry, I'm not actually sure why this fix is correct.I stole both the fix and the comment from a similar one on L150 of the module map left by Richard Smith.</div><div><br></div><div>/Eric</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 25, 2018 at 8:36 PM Shuai Wang <<a href="mailto:shuaiwang@google.com" target="_blank">shuaiwang@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I'd like to understand this better as well, in particular what would be a proper fix?</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 25, 2018 at 2:15 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">+Shuai Wang<br><br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 25, 2018 at 2:14 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hey Eric - thanks for the fix - but could you explain the issue here in a bit more detail, as I'm a bit confused (& really interested in understanding any layering problems in LLVM - and fixing them/making sure they're fixed/holding the line/etc)<br><br>What do you mean by "pull all of the AST matchers library into clang" - how does including a header ever add a link dependency?<br><br>- Dave</div><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sat, Sep 22, 2018 at 5:49 PM Eric Fiselier via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ericwf<br>
Date: Sat Sep 22 17:48:05 2018<br>
New Revision: 342827<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=342827&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=342827&view=rev</a><br>
Log:<br>
Fix modules build with shared library.<br>
<br>
r341994 caused clangAnalysis to pull all of the AST matchers<br>
library into clang. Due to inline key functions in the headers,<br>
importing the AST matchers library gives a link dependency on the<br>
AST matchers (and thus the AST), which clang should not<br>
have.<br>
<br>
This patch works around the issues by excluding the offending<br>
libclangAnalysis header in the modulemap.<br>
<br>
Modified:<br>
cfe/trunk/include/clang/module.modulemap<br>
<br>
Modified: cfe/trunk/include/clang/module.modulemap<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/module.modulemap?rev=342827&r1=342826&r2=342827&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/module.modulemap?rev=342827&r1=342826&r2=342827&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/module.modulemap (original)<br>
+++ cfe/trunk/include/clang/module.modulemap Sat Sep 22 17:48:05 2018<br>
@@ -5,6 +5,12 @@ module Clang_Analysis {<br>
textual header "Analysis/Analyses/ThreadSafetyOps.def"<br>
<br>
module * { export * }<br>
+<br>
+ // FIXME: Exclude these headers to avoid pulling all of the AST matchers<br>
+ // library into clang. Due to inline key functions in the headers,<br>
+ // importing the AST matchers library gives a link dependency on the AST<br>
+ // matchers (and thus the AST), which clang-format should not have.<br>
+ exclude header "Analysis/Analyses/ExprMutationAnalyzer.h"<br>
}<br>
<br>
module Clang_AST {<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></blockquote></div></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div></div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br></div></blockquote></div><br></div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>
</blockquote></div></div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></blockquote></div>