MaskRay accepted this revision. MaskRay added a comment. In D150549#4349531 <https://reviews.llvm.org/D150549#4349531>, @jobnoorman wrote:
> In D150549#4347056 <https://reviews.llvm.org/D150549#4347056>, @MaskRay wrote: > >> This seems fine, but please consider making `MC/SubtargetFeature.h` a >> forwarding header temporarily. > > To be clear: do you want me to check-in this forwarding header or should I > just do it locally for the tests below? If the former, what exactly is the > purpose of adding this header? The motivation is similar to https://reviews.llvm.org/D137838#3921828 > That way, you don't have to update oodles of include lines in this patch, and > it makes it a bit easier to see what's going on. (You can then update all the > include lines in a trivial follow-up if this change goes through, and then > remove the forwarding headers in Support, to cut the dependency you want to > remove.) (There is a non-zero probability that the patch may miss something and get reverted. By creating a forwarding header we don't risk causing too much churn to the many files.) For a large-scaling refactoring >> Our official build system CMake doesn't do layering checking for headers >> (https://llvm.org/docs/CodingStandards.html#library-layering). >> >> Our unofficial build system Bazel supports layering checking >> <http://maskray.me/blog/2022-09-25-layering-check-with-clang>. You may edit >> `utils/bazel/llvm-project-overlay/llvm/BUILD.bazel` and see whether bazel >> reports a circular dependency. I think this patch is fine, but just in case. > > I did the following: > > - Add the forwarding header > - Update `BUILD.bazel` (I only had to add the `TargetParser` dependency to > `llvm-tblgen` to make this work; should I add this change to this patch?) > - Run `bazel build '@llvm-project//llvm:all'` (using `clang` from `main`). > > This worked fine. Is this what you meant with the circular dependency check? > (I'm not familiar with bazel so I might be missing something.) Thank you for verification. Then I think this is good. `layering_check` is the default, so this verifies that we are good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150549/new/ https://reviews.llvm.org/D150549 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
