VOLK_VERSION is too hard to actually use.

[maitbot]

Maybe it would be better as a string.

The following code demonstrates that following the comments leads to unexpected results. // -----

include <volk/volk.h>

include

int main(int argc, char argv[]) { // No complaints yet... std::cout << "Using Volk: " << VOLK_VERSION_MAJOR << "." << VOLK_VERSION_MINOR << "." << VOLK_VERSION_MAINT << "\n"; /

• VOLK_VERSION % 100 is the MAINT version
• (VOLK_VERSION / 100) % 100 is the MINOR version
• (VOLK_VERSION / 100) / 100 is the MAJOR version / std::cout << "Using VOLK_VERSION: " << VOLK_VERSION << ": " << (VOLK_VERSION / 100)/100 << " " << (VOLK_VERSION /100) %100 << " " << (VOLK_VERSION % 100) << " ...Hmm. Not Decimal.\n"; // Uh... No. unsigned int v = VOLK_VERSION; std::cout << "It's octal: 0o0" << (v>>12)%8 << ((v>>9)%8)8 << ((v>>6)%8) << ((v>>3)%8)*8 << v%8 << " ?\n"; std::cout << "Or, base64: " << (v>>12)%64 << "." << ((v>>6)%64) << "." << v%64 << "\n"; // cout is interpreting these values as decimal, what could possibly go wrong? return(0); } // -----

Some observed output:

Using Volk: 3.0.0 Using VOLK_VERSION: 12288: 1 22 88 ...Hmm. Not Decimal. It's octal: 0o030000 ? Or, base64: 3.0.0.

Using Volk: 2.5.2 Using VOLK_VERSION: 8514: 0 85 14 ...Hmm. Not Decimal. It's octal: 0o020502 ? Or, base64: 2.5.2

So, yeah. Maybe revise code and/or comments in the include file, /usr/include/volk/volk_version.h

[maitbot]

Also note the recent observation via Python:

import ctypes lib = ctypes.cdll.LoadLibrary("libvolk.so") lib.volk_version()

as a reminder that making this better for C/C++ should make it better for Python bindings too.

[argilo]

The versioning header was added in #346, and it definitely looks like it was unintentional to make it octal by prefixing zeroes. As written, it will fail (error: invalid digit "8" in octal constant) as soon as any component of the version number reaches 8.

GNU Radio already uses #if VOLK_VERSION >= 030100 in a number of places (https://github.com/gnuradio/gnuradio/pull/6961), and it's possible that other projects already depend on the current format, so we'll have to be mindful of that if we make changes.

[argilo]

It looks like Boost was the inspiration for the format, but it gets things right by not prefixing the major version number with zeroes:

https://www.boost.org/doc/libs/1_83_0/boost/version.hpp

[argilo]

as a reminder that making this better for C/C++ should make it better for Python bindings too

VOLK doesn't have Python bindings, does it?

[jdemel]

This is a tricky bug. What if someone relies on a maximum VOLK version? Or if one does some computation with this number to determine compatibility?

[argilo]

What if someone relies on a maximum VOLK version?

The change I'm proposing keeps the version numbers in order, so that should continue working.

[argilo]

Or if one does some computation with this number to determine compatibility?

I'd be surprised if there were computations other than "greater than" or "less than", which will still work.

volk_version.h is a recent addition, so it's probably not used much yet.

[argilo]

I definitely agree that we should think carefully before making changes. An alternative would be to preserve the current octal format (which we could do in CMake by computing the octal digits individually) but things would get weird if any version component reached 8. For instance, 3.8.9 would be VOLK_VERSION 031011, and 8.0.0 would be VOLK_VERSION 0100000.

[maitbot]

Yes. #721 carries the least surprise and better matches the original comment.