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: > > > 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? > > 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? > > I'd have to go back to the developer who found it to get the exact details, > but my belief is that it's the inclusion more so than the static assertion > itself. FWIW, the dependency was happening when doing a shared libraries > build (I believe in release mode). I can dig up those details if you'd like, > but my concern is that even if the issue is the static assertion, including > the header file makes it easy for someone to accidentally use other > facilities from Sema.h under the mistaken belief they're fine to do so. > > > 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? > > That sounds like a very good idea to me! Okay. I sent out the RFC to llvm-dev. It was long overdue. Somebody just needed to volunteer, start the discussion, and get it over with. 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
