[llvm] r328123 - Reapply Support layering fixes.

[Justin Bogner via llvm-commits]

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.

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.

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.


>  //
>  //===----------------------------------------------------------------------===//
>  
> -#ifndef LLVM_SUPPORT_COMPILER_H