Add support for i386, s390x in Triple::getArchTypeForLLVMName.
Details
Details
Diff Detail
Diff Detail
Unit Tests
Unit Tests
Event Timeline
Comment Actions
Test? I'm a little surprised that changing the spelling of an arch didn't cause any failures.
Comment Actions
To my surprise too, it actually did. I updated the diff by adding x86_64 alongside x86-64.
Comment Actions
You still need a test for this change. It seems there isn't really an existing test for this API; it's used in llvm/unittests/MC/TargetRegistry.cpp, but that's testing the registry rather than the API.
Comment Actions
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.
Comment Actions
I have updated the patch by removing the case for x86_64. Although it looks like that's a known problem in LLVM (converting the arch-name to Triple::x86_64 and then back to string does not return the original name), this may warrant a separate fix; so that, should something break up, we may easily revert it, and not blame this change.
@probinson, considering that getArchTypeForLLVMName does not seem to be tested anywhere, and proposing a new one for this small change may not be worth it, could it make sense not to a provide a test at this time?
Comment Actions
My original point was that this change is so small and unrelated to anything at the moment that it would be better for whatever s390x or i386 change that needs it, to introduce it then, not beforehand.
So this review should be abandoned in favour of the actual s390x review that needs it, with this change in it.
Comment Actions
@probinson, sorry for late reply, it turns out that the sub-project, which this change was needed for, has been recently dismissed, so no longer needed. Also, we did not have any change of ours to provide, alongside the patch. That said, considering that the inverse function parseArch already supports s390x and the proposed change is fairly minimal (besides that getArchTypeForLLVMName is already tested via lookupTarget), I'm still wondering if part of the patch (at least the case adding s390x arch) could be beneficial. If you think it's not, I'll close the review.
Comment Actions
Honestly, I don't see why this commit would be a problem without tests, given there aren't tests for the rest. It seems like an obvious omission to me.
clang-format: please reformat the code
- .Case("aarch64", aarch64) - .Case("aarch64_be", aarch64_be) - .Case("aarch64_32", aarch64_32) - .Case("arc", arc) - .Case("arm64", aarch64) // "arm64" is an alias for "aarch64" - .Case("arm64_32", aarch64_32) - .Case("arm", arm) - .Case("armeb", armeb) - .Case("avr", avr) - .StartsWith("bpf", BPFArch)122 diff lines are omitted. See full path.