[Openmp-commits] [PATCH] D150549: Move SubtargetFeature.h from MC to TargetParser

[Fangrui Song via Phabricator via Openmp-commits]

MaskRay added a comment.

In D150549#4352174 <https://reviews.llvm.org/D150549#4352174>, @jobnoorman wrote:

> In D150549#4351614 <https://reviews.llvm.org/D150549#4351614>, @MaskRay wrote:
>
>> 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
>
> Thanks for the link! If I understand the discussion there correctly, the motivation for adding the forwarding header was to not have to update any `#include` sites directly in order to reduce the size of the patch. So what you are asking me to do is
>
> 1. Update this patch to have a forwarding header and //not// change `#include` sites;
> 2. Create a second patch that updates `#include`s and removes the forward. This patch would be committed at some point in the future once we're sure the move is fine.
>
> Is this correct?
>
> I do wonder if this is necessary in this case as the patch is much smaller than the one in the link you shared.

If we are certain this will not be reverted and cause churn, making the header switch in one single patch looks fine to me...

I have checked that this patch has migrated things that might be neglected: `unittest` directories, openmp,mlir,clang, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150549