Change VERSION macros to allow compares

[baconpaul]

1. Add a CLAP_VERSION_x_DIGIT which is just 1, 2, 3
2. Make CLAP_VERSION_x be the uint cast of that
3. Add a CLAP_VERSION_NUMBER which is MAJOR << 16 + MINOR << 8 + REV so you can do a #if CLAP_VERSION_NUMBER <= 0x010105
4. Update CMakeLists to use DIGIT definitions

[baconpaul]

OK I force pushed this rebased to next.

[abique]

Given how CLAP_VERSION_NUMBER is constructed, I think if we use a C++ compiler we could static_assert that both the minor and revision are less than 256.

[baconpaul]

Given how CLAP_VERSION_NUMBER is constructed, I think if we use a C++ compiler we could static_assert that both the minor and revision are less than 256.

Yea very good idea

the whole problem here stems from the explicit uint32 in the version ifdef which breaks pre processor math. That’s why I separated into digits and so on - but adding a bunch of static asserts to the test program is a great idea that I’ll do once you let me know if you want to do this or just maintain the version twice

[baconpaul]

Anyway I pushed a new version which responds to the review comments if we want to go this route. If we want to go another route it obviously won't :)

The far far easier version of this change is to just maintain a separate CLAP_VERSION_NUMBER as 0x010105L by hand in the header file and then use the static assert to fail to compile if you don't keep them in sync. If you prefer that version let me know and I'll submit a separate PR for that.

[abique]

If I understand well, the problem that you want to solve is:

#if CLAP_VERSION_GE(1, 1, 4)
# include <x.h>
#endif

I try something.

[abique]

#define A 5
#define B 6
#define C 8

#define CHECK_GE(X, Y, Z) ((X) > (A)) || ((X) == (A) && ((Y) > (B) || ((Y) == (B) && ((Z) >= (C)))))

#if CHECK_GE(7, 9, 9)
# error "Working!"
#else
# error "Not working :'("
#endif

clang -c tutu.c 
tutu.c:8:3: error: "Working!"
# error "Working!"
  ^
1 error generated.

[abique]

So what I'd suggest:

• remove the uint32_t cast, so we have instead: #define CLAP_VERSION_MAJOR 1
• define a macro CLAP_VERSION_GE(X, Y, Z)
• we don't need CLAP_VERSION_NUMBER

I think we solve all the macro issues with that. If we need further utilities we should be able to write them using static inline / constexpr functions.

What do you think?

[robbert-vdh]

What was the actual reason for wanting to check the CLAP version using the preprocessor @Arakula? I feel like this may be an X-Y problem. If you just want to check if an include exists, you can use the standard has_include(<header/name.h>) expression. And you can't keep all CLAP draft extensions around without symbol clashes anyways since draft 1, draft 2, draft 3, and the final version will likely use the same symbol names. So you'd need to vendor and modify those headers anyways.

[baconpaul]

So what I'd suggest:

• remove the uint32_t cast, so we have instead: #define CLAP_VERSION_MAJOR 1
• define a macro CLAP_VERSION_GE(X, Y, Z)
• we don't need CLAP_VERSION_NUMBER

I think we solve all the macro issues with that. If we need further utilities we should be able to write them using static inline / constexpr functions.

What do you think?

Isn’t the uint32 cast needed so it compiles properly in msvc when used in an initializer list?

obviiusky if that cast isn’t needed there’s way easier solutions. But I think it is for the clap version macros no?

also the solution here of a single long you compare with is what c++ compiler version, juce and vst3 all use today. But we can do something else sure!

[baconpaul]

What was the actual reason for wanting to check the CLAP version using the preprocessor @Arakula? I feel like this may be an X-Y problem. If you just want to check if an include exists, you can use the standard has_include(<header/name.h>) expression. And you can't keep all CLAP draft extensions around without symbol clashes anyways since draft 1, draft 2, draft 3, and the final version will likely use the same symbol names. So you'd need to vendor and modify those headers anyways.

If your claim is “there is never a reason to compile time check clap version” then (1) yes absolutely we don’t need this and (2) whoooeee I would ask you to show your work a bit more :)

[robbert-vdh]

I'm not saying that. At all. I'm saying that in the case of loading old draft extensions there's probably a better way to do this especially when considering symbol clashes.

[abique]

Isn’t the uint32 cast needed so it compiles properly in msvc when used in an initializer list?

obviiusky if that cast isn’t needed there’s way easier solutions. But I think it is for the clap version macros no?

We can put the cast into CLAP_VERSION_INIT also. If that doesn't work the github tests will catch it I suppose.

also the solution here of a single long you compare with is what c++ compiler version, juce and vst3 all use today. But we can do something else sure!

We already have the major, minor and revision defines, so if we add the number, it means that it has to be constructed by using major, minor and revision defines. For me it is important to have a single definition of the version and it has to be easy to parse using a regex. Extracting the version from 0x01010c seems more difficult to me, also it is ambiguous if hex should be used or if 0x010112 for minor=12. It could also be written as 1001012: Major * 1000000 + Minor * 1000 + Revision.

I like CLAP_VERSION_GE(1, 1, 4) because it doesn't require to learn how the version is encoded, the code speaks for itself. CLAP_VERSION_NUMBER >= 0x010104 is OK too.

also the solution here of a single long you compare with is what c++ compiler version, juce and vst3 all use today.

Yes the format 0x0000000 has been used widely, but mac also used __builtin_available(macOS 11.0, *) which looks a bit more modern style to me.

[Arakula]

What was the actual reason for wanting to check the CLAP version using the preprocessor @Arakula? I feel like this may be an X-Y problem. If you just want to check if an include exists, you can use the standard has_include(<header/name.h>) expression. And you can't keep all CLAP draft extensions around without symbol clashes anyways since draft 1, draft 2, draft 3, and the final version will likely use the same symbol names. So you'd need to vendor and modify those headers anyways.

I tried to send an answer per email, but somehow this doesn't seem to make its way to Github (at least not to this discussion), so here it is again (sorry if it appears twice in the end):

The actual reason can be found here: https://github.com/free-audio/clap/discussions/252

... and I'm afraid has_include() won't be enough.

For one, it's a C++17 thing, and I regularly work with older (and not necessarily C++) compilers. But that's just me ...

The other, more general reason is that V1.1.4 not only does away with 2 draft extensions, it also modifies one (track_info.h) - and to keep both versions supported means that the old header contents have to be kept in a modified form.

[baconpaul]

Isn’t the uint32 cast needed so it compiles properly in msvc when used in an initializer list? obviiusky if that cast isn’t needed there’s way easier solutions. But I think it is for the clap version macros no?

We can put the cast into CLAP_VERSION_INIT also. If that doesn't work the github tests will catch it I suppose.

also the solution here of a single long you compare with is what c++ compiler version, juce and vst3 all use today. But we can do something else sure!

We already have the major, minor and revision defines, so if we add the number, it means that it has to be constructed by using major, minor and revision defines. For me it is important to have a single definition of the version and it has to be easy to parse using a regex. Extracting the version from 0x01010c seems more difficult to me, also it is ambiguous if hex should be used or if 0x010112 for minor=12. It could also be written as 1001012: Major * 1000000 + Minor * 1000 + Revision.

I like CLAP_VERSION_GE(1, 1, 4) because it doesn't require to learn how the version is encoded, the code speaks for itself. CLAP_VERSION_NUMBER >= 0x010104 is OK too.

also the solution here of a single long you compare with is what c++ compiler version, juce and vst3 all use today.

Yes the format 0x0000000 has been used widely, but mac also used __builtin_available(macOS 11.0, *) which looks a bit more modern style to me.

So the pr I sent does constructs the number from major minor and revision right?

thenthing about ge is you need to define a le gt lt EQ ne version too which is why I think most systems define a hex constant the way the pr did?

thst built in available compiler intrinsic is an oddity of gcc and clang by the way (and actually caused surge for rack to fail to load on mac when using the initial rack cross compile environment even though it built).

But I’m happy with whatever you think is best! I just tossed in what I thought was a simple pr whcih didn’t change the cast on current ifdefs (so won’t break anything except regex parsers) and let us define a constant we can compare with. What do you think we should do?

[baconpaul]

I also pushed a change which has the minor version >= 0 not 1. Thanks!

[tim-janik]

I may be missing something here, but... Can't we simply do what is best practice in other C projects?

I.e. (adapting from Gtk+ here):

#define CLAP_VERSION_MAJOR    1
#define CLAP_VERSION_MINOR    1
#define CLAP_VERSION_REVISION 5
#define CLAP_CHECK_VERSION(major,minor,revision)    \
    (CLAP_VERSION_MAJOR > (major) || \
     (CLAP_VERSION_MAJOR == (major) && CLAP_VERSION_MINOR > (minor)) || \
     (CLAP_VERSION_MAJOR == (major) && CLAP_VERSION_MINOR == (minor) && \
      CLAP_VERSION_REVISION >= (revision)))

If a specific type is needed in some context, simply do:

auto version = { (uint) CLAP_VERSION_MAJOR, (uint) CLAP_VERSION_MINOR, (uint) CLAP_VERSION_REVISION };

And if some project relies on being able to check < 1.1.5 with the pre-processor (since we've already released <=1.1.4 version macros with the broken type cast), they can at least do:

#ifndef CLAP_CHECK_VERSION    // clap < 1.1.5

[baconpaul]

Yea there’s two patterns. The macro or the compiled constant. We should pick one. The macro is harder to do range comparisons and stuff (if version < 7 and > 608 is one we use in juce for instance)

And then decide if we want to strip the uint cast and potentially break people who rely on it in c or not

my choice was

• single constant like c++ juce and vst3
• Don’t strip the uint

Which is what the pr here does.

But happy to close this and someone to toss in an alternative

[robbert-vdh]

I tried to send an answer per email, but somehow this doesn't seem to make its way to Github (at least not to this discussion), so here it is again (sorry if it appears twice in the end):

The actual reason can be found here: #252

... and I'm afraid has_include() won't be enough.

For one, it's a C++17 thing, and I regularly work with older (and not necessarily C++) compilers. But that's just me ...

The other, more general reason is that V1.1.4 not only does away with 2 draft extensions, it also modifies one (track_info.h) - and to keep both versions supported means that the old header contents have to be kept in a modified form.

Isn't it much more maintainable to then always vendor the extensions? Instead of including either a vendored copy or the version from the CLAP headers depending on which CLAP version you're targetting?

---

And wrt version comparisons, I'd just define a couple comparison macros. Then you don't need to encode the CLAP version as a single integer, and the usage is much more readable once you go beyond to 0-9 range for major/minor/revision version numbers. A CLAP_VERSION_GE(x, y, z), CLAP_VERSION_LT(x, y, z), and a CLAP_VERSION_EQ(x, y, z) should cover most use cases, no?

[baconpaul]

Ok and what would you do with the uint casts?

[robbert-vdh]

I'd move them to the CLAP_VERSION_INIT macro.

[baconpaul]

I'd move them to the CLAP_VERSION_INIT macro.

OK! Should I mark that as a potentially breaking change in the changelog?

[robbert-vdh]

Yeah I guess it should be mentioned on the changelog since with that change the CLAP_VERSION_{MAJOR,MINOR,REVISION} macros are now plain integer literals instead of uint32_t expressions. Though in practice of course the odds of this breaking anything are very low of course.

[baconpaul]

OK well if you could re-review the change I think it now matches the macro style properly, has an appropriate change log, and tests the macros in the test builds.

[baconpaul]

by the way I notice the checks don't run on a pull request. Is that on purpose? I can toss in the yml to fix that on a separate PR if you want. [edit: oh but now i see the check has an odd vcpkg thing it does so i don't know if that will work. would be great though if the PR could run the compiles for me!]

[abique]

by the way I notice the checks don't run on a pull request. Is that on purpose? I can toss in the yml to fix that on a separate PR if you want. [edit: oh but now i see the check has an odd vcpkg thing it does so i don't know if that will work. would be great though if the PR could run the compiles for me!]

This is bad, I'll look into why.

[baconpaul]

by the way I notice the checks don't run on a pull request. Is that on purpose? I can toss in the yml to fix that on a separate PR if you want. [edit: oh but now i see the check has an odd vcpkg thing it does so i don't know if that will work. would be great though if the PR could run the compiles for me!]

This is bad, I'll look into why.

Oh the reason why is because the trigger is [push, workflow_request], not [push, pull_request, workflow_request] in the .yml

[abique]

Oh the reason why is because the trigger is [push, workflow_request], not [push, pull_request, workflow_request] in the .yml

I've added it

[baconpaul]

Oh the reason why is because the trigger is [push, workflow_request], not [push, pull_request, workflow_request] in the .yml

I've added it

OK let me rebase this then and we can see if they run.

[baconpaul]

Great and with the rebase you can see the builds running! Super.

[abique]

Thanks!