<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jun 4, 2018 at 8:01 PM David Blaikie via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">(+Chandler since he had some feedback on another layering change anyway)<br><br><div class="gmail_quote"></div></div><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Jun 1, 2018 at 2:31 PM Justin Bogner <<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">David Blaikie via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> writes:<br>
> Author: dblaikie<br>
> Date: Wed Mar 21 10:31:49 2018<br>
> New Revision: 328123<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=328123&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=328123&view=rev</a><br>
> Log:<br>
> Reapply Support layering fixes.<br>
><br>
> Compiler.h is used by Demangle (which Support depends on) - so sink it<br>
> into Demangle to avoid a circular dependency<br>
<br>
Sorry for seeing this a bit late, but this fix seems very wrong to me.<br>
<br>
> DataTypes.h is used by llvm-c (which Support depends on) - so sink it<br>
> into llvm-c.<br>
><br>
> DataTypes.h could probably be fixed the other way - making llvm-c depend<br>
> on Support instead of Support depending on llvm-c - if anyone feels<br>
> that's the better option, happy to work with them on that.<br>
><br>
> I /think/ this'll address the layering issues that previous attempts to<br>
> commit this have triggered in the Modules buildbot, but I haven't been<br>
> able to reproduce that build so can't say for sure. If anyone's having<br>
> trouble with this - it might be worth taking a look to see if there's a<br>
> quick fix/something small I missed rather than revert, but no worries.<br>
><br>
> Copied: llvm/trunk/include/llvm/Demangle/Compiler.h (from r328122, llvm/trunk/include/llvm/Support/Compiler.h)<br>
> URL: <a href="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" rel="noreferrer" target="_blank">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</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/Support/Compiler.h (original)<br>
> +++ llvm/trunk/include/llvm/Demangle/Compiler.h Wed Mar 21 10:31:49 2018<br>
> @@ -1,4 +1,4 @@<br>
> -//===-- llvm/Support/Compiler.h - Compiler abstraction support --*- C++ -*-===//<br>
> +//===-- llvm/Demangle/Compiler.h - Compiler abstraction support -*- C++ -*-===//<br>
>  //<br>
>  //                     The LLVM Compiler Infrastructure<br>
>  //<br>
><br>
> Modified: llvm/trunk/include/llvm/Support/Compiler.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Compiler.h?rev=328123&r1=328122&r2=328123&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Compiler.h?rev=328123&r1=328122&r2=328123&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/Support/Compiler.h (original)<br>
> +++ llvm/trunk/include/llvm/Support/Compiler.h Wed Mar 21 10:31:49 2018<br>
> @@ -7,500 +7,13 @@<br>
>  //<br>
>  //===----------------------------------------------------------------------===//<br>
>  //<br>
> -// This file defines several macros, based on the current compiler.  This allows<br>
> -// use of compiler-specific features in a way that remains portable.<br>
> +// Due to layering constraints (Support depends on Demangler) this is a thin<br>
> +// wrapper around the implementation that lives in llvm-c, though most clients<br>
> +// can/should think of this as being provided by Support for simplicity (not<br>
> +// many clients are aware of their dependency on Demangler/it's a weird place to<br>
> +// own this - but didn't seem to justify splitting Support into "lower support"<br>
> +// and "upper support").<br>
<br>
Demangler isn't only a "weird place" to own this, it's a complete break<br>
in abstraction. None of this stuff has to do with demangler at all, so<br>
sinking it there because demangler happens to depend in it is purely a<br>
hack. I feel like the fact that we left a shim header in Compiler.h to<br>
paper over it is pretty strong evidence that you agree here.<br></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>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.<br> </div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I assume you considered moving Signals.h (the only thing that depends on<br>
Demangle) out of Support and rejected that idea for one reason or<br>
another, but that's obviously one option for doing this more cleanly.<br></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>Hadn't looked at that option, really, though... <br><br>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.<br><br>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.<br> </div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">If that's problematic for some reason, I think a pretty strong argument<br>
could be made for moving Compiler.h to Config/ - it's header only and<br>
really only exists to paper over differences in the build configuration.<br></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>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.<br></div></div></div></blockquote><div><br></div><div>I don't think this really helps.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
>  //<br>
>  //===----------------------------------------------------------------------===//<br>
>  <br>
> -#ifndef LLVM_SUPPORT_COMPILER_H<br>
</blockquote></div></div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>