Shuai - have you had a chance to look at this? On Mon, Oct 22, 2018 at 4:43 PM Richard Smith <rich...@metafoo.co.uk> wrote:
> On Mon, 22 Oct 2018 at 14:57, David Blaikie via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> 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. >> > > Here's the intended current directory layout and layering as I understand > it: > > * 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 > * 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 > * the StaticAnalyzer library should contain all the pieces that are > specific to the static analyzer > * Sema should not depend on Tooling, but it's fine for StaticAnalyzer to > depend on Tooling > > Compared to where we are today, there are two differences: > > 1) The implementations of the headers in include/clang/Analysis are in > lib/Analysis, not in lib/Analysis/Analyses > 2) ExprMutationAnalyzer is in lib/Analysis despite not being part of the > common infrastructure depended on by Sema and StaticAnalyzer > > For point 1, we should change the lib/Analysis directory layout to match > the headers. > 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. > > On Tue, Oct 2, 2018 at 2:44 PM Shuai Wang via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> On Mon, Oct 1, 2018 at 4:58 PM Richard Smith <rich...@metafoo.co.uk> >>> wrote: >>> >>>> On Mon, 1 Oct 2018 at 16:10, George Karpenkov via cfe-commits < >>>> cfe-commits@lists.llvm.org> wrote: >>>> >>>>> Hi Richard, >>>>> >>>>> On Oct 1, 2018, at 2:50 PM, Richard Smith via cfe-commits < >>>>> cfe-commits@lists.llvm.org> wrote: >>>>> >>>>> 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. >>>>> >>>>> Shuai, what is the goal here? Why is this code being moved to >>>>> Analysis/? >>>>> >>>>> >>>>> I’ve asked for this possibility, as I wanted to use it from the Clang >>>>> static analyzer. >>>>> >>>>> 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? >>>>> >>>>> We have historically decided to not use the tooling interfaces >>>>> (ASTMatcher, ParentMap, etc) from the frontend, >>>>> >>>>> >>>>> Clang analyzer uses ASTMatcher all over the place. >>>>> >>>>> 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) >>>>> >>>>> >>>>> That’s totally good for the analyzer though, right? >>>>> >>>> >>>> Yes, that all seems fine for the static analyzer. >>>> >>>> >>>>> In any case, in future it might make sense to move the analyzer out of >>>>> Clang proper. >>>>> But for know the only way to use clang-tidy utilities from the >>>>> analyzer is to move them into Clang. >>>>> >>>> >>>> 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) >>>> >>>> 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. >>>> >>> I'm happy to do the move. >>> Could you (or someone) help point out where exactly I should move it to >>> though? >>> >>>> . If you want to change that, we'll need to discuss that decision. >>>>> >>>>> 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? >>>>> >>>>> >>>>> On Mon, 1 Oct 2018 at 13:13, David Blaikie via cfe-commits < >>>>> cfe-commits@lists.llvm.org> wrote: >>>>> >>>>>> 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. >>>>>> >>>>>> So, I'm wondering what error you hit, Eric/where/how, etc... >>>>>> >>>>>> On Sun, Sep 30, 2018 at 10:23 AM Eric Fiselier <e...@efcs.ca> wrote: >>>>>> >>>>>>> +rsmith (actually this time) >>>>>>> >>>>>>> On Sun, Sep 30, 2018 at 12:09 PM Eric Fiselier <e...@efcs.ca> wrote: >>>>>>> >>>>>>>> +rsmith >>>>>>>> >>>>>>>> Hi All, >>>>>>>> >>>>>>>> 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. >>>>>>>> >>>>>>>> /Eric >>>>>>>> >>>>>>>> On Tue, Sep 25, 2018 at 8:36 PM Shuai Wang <shuaiw...@google.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> I'd like to understand this better as well, in particular what >>>>>>>>> would be a proper fix? >>>>>>>>> >>>>>>>>> On Tue, Sep 25, 2018 at 2:15 PM David Blaikie <dblai...@gmail.com> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> +Shuai Wang >>>>>>>>>> >>>>>>>>>> On Tue, Sep 25, 2018 at 2:14 PM David Blaikie <dblai...@gmail.com> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> 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) >>>>>>>>>>> >>>>>>>>>>> What do you mean by "pull all of the AST matchers library into >>>>>>>>>>> clang" - how does including a header ever add a link dependency? >>>>>>>>>>> >>>>>>>>>>> - Dave >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Sat, Sep 22, 2018 at 5:49 PM Eric Fiselier via cfe-commits < >>>>>>>>>>> cfe-commits@lists.llvm.org> wrote: >>>>>>>>>>> >>>>>>>>>>>> Author: ericwf >>>>>>>>>>>> Date: Sat Sep 22 17:48:05 2018 >>>>>>>>>>>> New Revision: 342827 >>>>>>>>>>>> >>>>>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=342827&view=rev >>>>>>>>>>>> Log: >>>>>>>>>>>> Fix modules build with shared library. >>>>>>>>>>>> >>>>>>>>>>>> r341994 caused clangAnalysis to pull all of the AST matchers >>>>>>>>>>>> library into clang. Due to inline key functions in the headers, >>>>>>>>>>>> importing the AST matchers library gives a link dependency on >>>>>>>>>>>> the >>>>>>>>>>>> AST matchers (and thus the AST), which clang should not >>>>>>>>>>>> have. >>>>>>>>>>>> >>>>>>>>>>>> This patch works around the issues by excluding the offending >>>>>>>>>>>> libclangAnalysis header in the modulemap. >>>>>>>>>>>> >>>>>>>>>>>> Modified: >>>>>>>>>>>> cfe/trunk/include/clang/module.modulemap >>>>>>>>>>>> >>>>>>>>>>>> Modified: cfe/trunk/include/clang/module.modulemap >>>>>>>>>>>> URL: >>>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/module.modulemap?rev=342827&r1=342826&r2=342827&view=diff >>>>>>>>>>>> >>>>>>>>>>>> ============================================================================== >>>>>>>>>>>> --- cfe/trunk/include/clang/module.modulemap (original) >>>>>>>>>>>> +++ cfe/trunk/include/clang/module.modulemap Sat Sep 22 >>>>>>>>>>>> 17:48:05 2018 >>>>>>>>>>>> @@ -5,6 +5,12 @@ module Clang_Analysis { >>>>>>>>>>>> textual header "Analysis/Analyses/ThreadSafetyOps.def" >>>>>>>>>>>> >>>>>>>>>>>> module * { export * } >>>>>>>>>>>> + >>>>>>>>>>>> + // FIXME: Exclude these headers to avoid pulling all of the >>>>>>>>>>>> AST matchers >>>>>>>>>>>> + // library into clang. Due to inline key functions in the >>>>>>>>>>>> headers, >>>>>>>>>>>> + // importing the AST matchers library gives a link >>>>>>>>>>>> dependency on the AST >>>>>>>>>>>> + // matchers (and thus the AST), which clang-format should >>>>>>>>>>>> not have. >>>>>>>>>>>> + exclude header "Analysis/Analyses/ExprMutationAnalyzer.h" >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> module Clang_AST { >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>> cfe-commits mailing list >>>>>>>>>>>> cfe-commits@lists.llvm.org >>>>>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>>>>>>>>> >>>>>>>>>>> _______________________________________________ >>>>>> cfe-commits mailing list >>>>>> cfe-commits@lists.llvm.org >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> cfe-commits@lists.llvm.org >>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> cfe-commits@lists.llvm.org >>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits