Don't show compiler bug message on invalid target machine #4847
Conversation
| @@ -19,7 +22,7 @@ module Crystal | |||
| features += "+vfp2" | |||
| end | |||
| else | |||
| raise "Unsupported arch for target triple: #{target_triple}" | |||
| raise TargetMachine::Error.new("Unsupported architecture for target triple: #{target_triple} (only x86, x86_64, and ARM are supported)") | |||
There was a problem hiding this comment.
I'm not sure if the supported architectures should be hardcoded in the error message. Might be missed when (and if) say PowerPC would be supported someday.
Maybe just provide a link to https://github.com/crystal-lang/crystal/wiki/Platform-Support (the wiki page is still a draft though).
There was a problem hiding this comment.
I don't really like linking to places from compiler error messages, it feels wrong. Perhaps we'll have to remove this message once we implement more than 5 architectures but I think it's fine for now.
There was a problem hiding this comment.
How about adding a method with a maintained (or generated) list of supported targets, and use that instead ?
There was a problem hiding this comment.
Or just a note to look up supported architectures in the documentation (given it's available there).
There was a problem hiding this comment.
We shouldn't list supported architectures, it's not required and could be confusing or even wrong; it's possible we added support but didn't change the message, it's possible LLVM wasn't built with a supported architectures, for example homebrew doesn't build the ARM and AArch64 targets.
There was a problem hiding this comment.
@ysbaddaden Just a note: In the case that the specific LLVM build does not include some otherwise supported target, it would not result in an error in this method.
There was a problem hiding this comment.
Ok, I see your point, I'll remove the list of supported architectures.
RX14
force-pushed
the
feature/bugfix/unsupported-arch-error
branch
from
276fe4b
to
0542dfc
Compare
Fixes #4842.
I've also done a bit of refactoring around of the locationless compiler errors, hopefully my refactor makes sense.