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

Fix version check for LLVM < 6.0 #6383

Merged
merged 1 commit into from
Jul 28, 2018

Conversation

felixbuenemann
Copy link
Contributor

This is a minor fixup for the LLVM 6.0 support quick fix (#6380) that changes the versions check for "below 6.0" (<= 5.9) to <= 5.0 since there wont be any new 5.x versions after 5.0.

This is a minor fixup for the LLVM 6.0 support quick fix that changes
the versions check for "below 6.0" (<= 5.9) to <= 5.0 since there wont
be any new 5.x versions after 5.0.
@RX14
Copy link
Contributor

RX14 commented Jul 14, 2018

Not sure this is worth merging, it works fine as-is.

@ilovezfs
Copy link

http://blog.llvm.org/2016/12/llvms-new-versioning-scheme.html

Patch releases will increment the patch component as before (producing versions like 5.0.1), and the minor component will stay at zero since no minor releases will be made.

@RX14
Copy link
Contributor

RX14 commented Jul 14, 2018

This is either an incorrect PR or a purely-aesthetic change, depending on the implementation of LLVM_VERSION_LE. I can't be bothered to check which one it is, because it doesn't matter. I vote close.

@ilovezfs
Copy link

Why not change the 9 to a prettier number like 42?

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Let's fixup. It fixes an imaginary version number, to one that does exist.

@felixbuenemann
Copy link
Contributor Author

This is either an incorrect PR or a purely-aesthetic change, depending on the implementation of LLVM_VERSION_LE.

I can assure you it is not incorrect, the previous code was.

The LLVM_VERSION_LE macro only checks the major and minor version of LLVM. After LLVM 5.0 there is 6.0 and the next version will be 7.0. There are patch releases like 5.0.1/5.0.2, but there will never be a 5.1 release.

The only reason I initially used 5.9 as a check, is because I wanted to write "lower than 6.0" and then noticed the macro was lower-than-or-equal so I change it from 6.0 to 5.9, which technically works, but is confusing to the reader of the code.

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @felixbuenemann 👍

@sdogruyol sdogruyol merged commit 9b32f7d into crystal-lang:master Jul 28, 2018
@sdogruyol sdogruyol added this to the Next milestone Jul 28, 2018
@felixbuenemann felixbuenemann deleted the llvm-6.0-support branch July 29, 2018 07:32
@RX14 RX14 modified the milestones: Next, 0.26.0 Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants