[PATCH] D122003: [Triple] Add support for more archs in `getArchTypeForLLVMName`

Renato Golin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 22 03:27:59 PDT 2022


rengolin added a comment.

This is already tested by its users (via `lookupTarget`) in all sorts of places, but not this change in particular, which is the problem.

But I think testing this as an API (by checking that all names resolve to the correct arch) is too trivial to be worth doing it.

So I recommend you submit the actual change you need this for, and only what you need from this patch on it. If the new change has tests (compiling, linking) on the arch you need, than that'll be the test.

It's unclear to me why you have to add `x86_64` after adding new cases to the switch, so I suggest you don't add more than what you need on your actual patch.

I think once we know where this is coming from and why you need it, and you have tests for that, we'll be comfortable in knowing this is being tested.


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

https://reviews.llvm.org/D122003



More information about the llvm-commits mailing list