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