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: > > 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. Thank you for your help on this! 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
