r342827 - Fix modules build with shared library.

[Richard Smith via cfe-commits]

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/? 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, 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). 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 at 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 <eric at efcs.ca> wrote:
>
>> +rsmith (actually this time)
>>
>> On Sun, Sep 30, 2018 at 12:09 PM Eric Fiselier <eric at 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 <shuaiwang at 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 <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>> +Shuai Wang
>>>>>
>>>>> On Tue, Sep 25, 2018 at 2:14 PM David Blaikie <dblaikie at 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 at 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 at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>>
>>>>>> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181001/894741c0/attachment.html>