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

Express version information in decimal #721

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

argilo
Copy link
Member

@argilo argilo commented Dec 11, 2023

Fixes #720.

VOLK's version header (created in #346) unintentionally expresses version numbers in octal, because it prefixes the (decimal) numbers with zeroes. This will fail (with error: invalid digit "8" in octal constant) as soon as any component of the version number reaches 8.

The documentation in the header states that VOLK_VERSION is decimal, and this is a more convenient format, so I think it makes sense to switch to that. GNU radio already uses #if VOLK_VERSION >= 030100, but this condition will continue to be true if VOLK switches to decimal because 3010010 is greater than 0301008 (= 1235210).

I considered instead changing the header documentation to indicate that the format is octal, but it would be difficult to implement correct header generation in CMake, because CMake's math only supports decimal and hexadecimal output formats.

@argilo
Copy link
Member Author

argilo commented Dec 11, 2023

Before:

#define VOLK_VERSION_MAJOR 03
#define VOLK_VERSION_MINOR 01
#define VOLK_VERSION_MAINT 00

[...]

#define VOLK_VERSION 030100

After:

#define VOLK_VERSION_MAJOR 3
#define VOLK_VERSION_MINOR 1
#define VOLK_VERSION_MAINT 0

[...]

#define VOLK_VERSION 30100

@argilo
Copy link
Member Author

argilo commented Dec 11, 2023

GNSS-SDR is the one other user of VOLK that I know of, and it doesn't use the VOLK_VERSION macros.

Copy link
Member

@marcusmueller marcusmueller 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 do this, before the current state causes any more damage.

Copy link
Contributor

@jdemel jdemel left a comment

Choose a reason for hiding this comment

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

LGTM

@jdemel jdemel merged commit f2fb33c into gnuradio:main Dec 17, 2023
32 checks passed
@jdemel
Copy link
Contributor

jdemel commented Dec 17, 2023

@argilo
Copy link
Member Author

argilo commented Dec 17, 2023

This code might be affected

That will continue working, for the same reason that GNU Radio's checks will. Future decimal versions numbers will be greater than 0301008.

@argilo argilo deleted the fix-volk-version branch December 17, 2023 13:22
Alesha72003 pushed a commit to Alesha72003/volk that referenced this pull request May 15, 2024
Express version information in decimal
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.

VOLK_VERSION is too hard to actually use.
3 participants