[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"
----------------
davezarzycki wrote:
> aaron.ballman wrote:
> > rnk wrote:
> > > This includes Sema.h into every codegen file that uses CGValue.h (most of them). That seems bad for build time. :(
> > > 
> > > This also seems like a layering violation. CodeGen has no dependency on Sema:
> > > https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104
> > I agree that this is a layering violation (Sema relies on CodeGen which now relies on Sema due to this change). We just ran into it in a downstream fork when we had to add `clangSema` to the codegen linker input to avoid linking errors. I'm a bit surprised given that the only use appears to be a `static_assert` that shouldn't require anything to be linked in, but here we are just the same.
> > 
> > I think this should be rolled back so that we don't get additional dependencies on Sema within CodeGen by accident. It helps that @rnk moved this change into CGDecl.cpp (limits the scope of where we may introduce accidental dependencies), but I don't think we should be including anything from Sema.h here.
> It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13
That fixes the transitive include issues @rnk raised, but is not a fix for the layering violation. It's still a layering violation because it's including Sema from within CodeGen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73363/new/

https://reviews.llvm.org/D73363