Sorry for the unresponsiveness, I've been fire fighting for other stuffs. I'll look into this and try to get it fixed this week.
On Tue, Nov 6, 2018 at 9:52 AM David Blaikie <dblai...@gmail.com> wrote: > 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
