davezarzycki 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" ---------------- aaron.ballman wrote: > 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. Ah, sorry. It does seem odd and probably a bug that a static_assert would trigger this. Did you verify that the imported symbol dependency is triggered by this static_assert? Or is the problem the mere inclusion, not the static_assert itself? And is the actual dependency only happen in unoptimized/debug builds or release too? In any case, I believe it was discussed at the time that the LLVM variable ought to be moved to some kind of policy/config header that both LLVM's CodeGen and clang's Sema can include. Would you be open to that? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73363/new/ https://reviews.llvm.org/D73363 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
