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

[Job Noorman via Phabricator via Openmp-commits]

jobnoorman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150549