r342827 - Fix modules build with shared library.

Mon Oct 1 15:03:36 PDT 2018

George wanted to use it from clang (or, at least can't use from anything
from clang-tidy.) I was actually assuming the intended usage is from some
tooling instead of the frontend itself, but I never checked with George.
Personally I don't think ExprMutationAnalyzer should be used in anywhere
other than toolings. I'm happy to move it around if current location is not
desirable.

On Mon, Oct 1, 2018 at 2:50 PM Richard Smith <richard at metafoo.co.uk> 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/?
> 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/e15ea2fd/attachment.html>