Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't show compiler bug message on invalid target machine #4847

Merged

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Aug 18, 2017

Fixes #4842.

I've also done a bit of refactoring around of the locationless compiler errors, hopefully my refactor makes sense.

@@ -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)")
Copy link
Member

@straight-shoota straight-shoota Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@RX14 RX14 Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bew bew Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a method with a maintained (or generated) list of supported targets, and use that instead ?

Copy link
Member

@straight-shoota straight-shoota Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just a note to look up supported architectures in the documentation (given it's available there).

Copy link
Contributor

@ysbaddaden ysbaddaden Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@straight-shoota straight-shoota Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@RX14 RX14 Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see your point, I'll remove the list of supported architectures.

Copy link
Contributor Author

@RX14 RX14 Aug 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@RX14 RX14 force-pushed the feature/bugfix/unsupported-arch-error branch from 276fe4b to 0542dfc Compare Aug 20, 2017
@ysbaddaden ysbaddaden merged commit e844983 into crystal-lang:master Aug 28, 2017
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants