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.
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