[PATCH] D52149: add support functions for hard/soft float multilib distinction

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

In https://reviews.llvm.org/D52149#1250735, @kristina wrote:

> I think it's okay to land this for convenience for now (once the issues are addressed) and if the Clang differential does not get approved revert this, which is not especially difficult since the support lib is quite isolated.


This is a re-implementation of existing functionality in a completely different part of the code. With time, both implementations can diverge, creating hard-to-spot problems. He had enough of those already between Clang and LLVM (see TargetParser) and LLVM itself (see isLikeA9).

Convenience today usually means disaster tomorrow. :)

> The core issue is in whether the diff that depends on this get approved or not, but I think since it does no harm and is not likely to be used by anything else,

The harm it does is code duplication, and not being used by anything else is actually a problem, not a feature.

> it would be okay to land for now and revert it if the Clang change is not approved. In either case this is better as a separate diff rather than a part of the Clang-specific diff, because of its isolation.

Committing a patch with the intent to revert is never a good policy.

Also, because we still have repo separation, we *have* to have two separate patches, one for Clang and one for LLVM. But we tend to tie them up in a patch series and commit one after the other, to make sure bisects don't suffer too greatly.

It would be a better strategy to make use of the existing functionality, or to propose a way to either expose that functionality (if not visible from Clang) or to move it to a different place.

Duplicating it would be a bad idea in any case.


Repository:
  rL LLVM

https://reviews.llvm.org/D52149