[llvm] [llvm] Add format check for MCSubtargetFeatures (PR #180943)

[David Spickett via llvm-commits]

DavidSpickett wrote:

Preface: This is a wall of text but after several attempts, this is the clearest way I can put it all. Amount of text is not correlated to anything really, and I do not think anyone here is acting in bad faith in any way.

* This PR lacks justification for why the validation is so strict. (trailing comma hardly seems like the end of the world)
* This PR does not define "inconsistent". With what? With the user's expectations, with the architecture spec?
* This PR lacks a statement about the use of this feature string. Whether it is purely internal, external, expected to be used from downstream forks, etc. What's the impact of suddenly rejecting these "invalid" formats?
* Without that justification, it looks like it is overly strict and pushing the minutia of the format onto all callers of this API, like LLDB. Which smells like a layering problem.
* The error it returns will mean nothing to anyone who gets it. Though it is no worse than the other failure modes to be fair, and sometimes "no worse" is a winning argument, but not one that I see being made in the PR description.
* The idea that all callers of this API will make a nice error that users can act on, again, smells like a layering violation. Also because two places return nullptr, you can't tell what the failure reason was.
* If I get this nullptr, what do I do about it? Am I supposed to read llvm source code to find out what the format is? There's doxygen but:
  ```
  /// \param Features This specifies the string representation of the
  /// additional target features.
  ```
  It basically says "the features string is the features string in the features string format everyone knows to use for features strings". At the very least I need a noun that I can search Google or AI or the codebase for.
* If the format is already documented and this is just enforcing it, how is anyone supposed to get from `nullptr` to said documentation?

I've no doubt you have well thought out answers to all of these, but I do not see any of that thought written down in this PR.

It seems like there's a desire for this change, from people I respect the opinions of. So I'm not going to block it myself, but I would really like to make sure that people's reasons for being positive about this PR, are in fact consistent. If we accept PRs based on assumed justifications, it's so much harder to maintain the code later, or help users with the fallout of code changes.

If my downstream tools break after this change, it would be really nice if when I track it back here, I see an explanation of why the change was made. I might disagree, but at least I'd understand and have a good idea how I can best adapt to the new reality.

> To be honest, I don't understand what you offer to change in comment in that PR.

Extend the description to say why you want this change, why it must be so strict, what the impacts of being strict are, why we cannot permissively parse,  what these inconsistent objects are and what their impact is.

https://github.com/llvm/llvm-project/pull/180943