[llvm] r328123 - Reapply Support layering fixes.

Mon Jun 4 11:11:01 PDT 2018

On Mon, Jun 4, 2018 at 8:01 PM David Blaikie via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> (+Chandler since he had some feedback on another layering change anyway)
>
> On Fri, Jun 1, 2018 at 2:31 PM Justin Bogner <mail at justinbogner.com>
> wrote:
>
>> David Blaikie via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> > Author: dblaikie
>> > Date: Wed Mar 21 10:31:49 2018
>> > New Revision: 328123
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=328123&view=rev
>> > Log:
>> > Reapply Support layering fixes.
>> >
>> > Compiler.h is used by Demangle (which Support depends on) - so sink it
>> > into Demangle to avoid a circular dependency
>>
>> Sorry for seeing this a bit late, but this fix seems very wrong to me.
>>
>> > DataTypes.h is used by llvm-c (which Support depends on) - so sink it
>> > into llvm-c.
>> >
>> > DataTypes.h could probably be fixed the other way - making llvm-c depend
>> > on Support instead of Support depending on llvm-c - if anyone feels
>> > that's the better option, happy to work with them on that.
>> >
>> > I /think/ this'll address the layering issues that previous attempts to
>> > commit this have triggered in the Modules buildbot, but I haven't been
>> > able to reproduce that build so can't say for sure. If anyone's having
>> > trouble with this - it might be worth taking a look to see if there's a
>> > quick fix/something small I missed rather than revert, but no worries.
>> >
>> > Copied: llvm/trunk/include/llvm/Demangle/Compiler.h (from r328122,
>> llvm/trunk/include/llvm/Support/Compiler.h)
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Demangle/Compiler.h?p2=llvm/trunk/include/llvm/Demangle/Compiler.h&p1=llvm/trunk/include/llvm/Support/Compiler.h&r1=328122&r2=328123&rev=328123&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/include/llvm/Support/Compiler.h (original)
>> > +++ llvm/trunk/include/llvm/Demangle/Compiler.h Wed Mar 21 10:31:49 2018
>> > @@ -1,4 +1,4 @@
>> > -//===-- llvm/Support/Compiler.h - Compiler abstraction support --*-
>> C++ -*-===//
>> > +//===-- llvm/Demangle/Compiler.h - Compiler abstraction support -*-
>> C++ -*-===//
>> >  //
>> >  //                     The LLVM Compiler Infrastructure
>> >  //
>> >
>> > Modified: llvm/trunk/include/llvm/Support/Compiler.h
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Compiler.h?rev=328123&r1=328122&r2=328123&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/include/llvm/Support/Compiler.h (original)
>> > +++ llvm/trunk/include/llvm/Support/Compiler.h Wed Mar 21 10:31:49 2018
>> > @@ -7,500 +7,13 @@
>> >  //
>> >
>> //===----------------------------------------------------------------------===//
>> >  //
>> > -// This file defines several macros, based on the current compiler.
>> This allows
>> > -// use of compiler-specific features in a way that remains portable.
>> > +// Due to layering constraints (Support depends on Demangler) this is
>> a thin
>> > +// wrapper around the implementation that lives in llvm-c, though most
>> clients
>> > +// can/should think of this as being provided by Support for
>> simplicity (not
>> > +// many clients are aware of their dependency on Demangler/it's a
>> weird place to
>> > +// own this - but didn't seem to justify splitting Support into "lower
>> support"
>> > +// and "upper support").
>>
>> Demangler isn't only a "weird place" to own this, it's a complete break
>> in abstraction. None of this stuff has to do with demangler at all, so
>> sinking it there because demangler happens to depend in it is purely a
>> hack. I feel like the fact that we left a shim header in Compiler.h to
>> paper over it is pretty strong evidence that you agree here.
>>
>
> Ish. I'm not feeling all that principled or worried about these layering
> fixes - it doesn't feel like this breaks anything in a huge way, though it
> doesn't feel super great either - though introducing new libraries to
> support the variations of layering that are required might be more hassle
> than its worth too.
>
>
>> I assume you considered moving Signals.h (the only thing that depends on
>> Demangle) out of Support and rejected that idea for one reason or
>> another, but that's obviously one option for doing this more cleanly.
>>
>
> Hadn't looked at that option, really, though...
>
> Looks like that'd have the same problem - since Signals.h is included from
> a bunch of places (fewer than Compiler.h, but still tens) & similarly has
> nothing to do with the Demangler.
>
> Or were you suggesting moving it somewhere else? Where? & its
> implementation (lib/Support/Signals.cpp) depends on several Support headers
> - so it couldn't be easily layered below Support by the looks of it.
>
>
>> If that's problematic for some reason, I think a pretty strong argument
>> could be made for moving Compiler.h to Config/ - it's header only and
>> really only exists to paper over differences in the build configuration.
>>
>
> We currently don't have any plain headers in Config - so I'm not sure if
> that'd work straight off the bat, but don't mind moving it there - is it
> worth renaming everything that includes "Support/Compiler.h" to include
> "Config/Compiler.h" though? Does that feel better/more right? I'm not
> really sure/don't much mind.
>

I don't think this really helps.

I think that if we want Demangle to not depend on Support, we should make
it not depend on Support --> remove all uses of constructs from Compiler.h
and directly use the language /compiler primitives.

Fundamentally, if Demangle needs to be "stand alone", we need to make that
100% true. If we don't then it should just be merged into Support itself.

>
>
>>
>>
>> >  //
>> >
>> //===----------------------------------------------------------------------===//
>> >
>> > -#ifndef LLVM_SUPPORT_COMPILER_H
>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180604/78ecae3e/attachment.html>