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