[llvm] r328123 - Reapply Support layering fixes.

[David Blaikie via llvm-commits]

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


>
>
> >  //
> >
> //===----------------------------------------------------------------------===//
> >
> > -#ifndef LLVM_SUPPORT_COMPILER_H
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180604/d3ce78db/attachment-0001.html>