Skip to content

Define OTP_VSN/OTP_RELEASE/HAS_CALLBACKS macro#348

Closed
iilyak wants to merge 1 commit intoapache:masterfrom
cloudant:set_features
Closed

Define OTP_VSN/OTP_RELEASE/HAS_CALLBACKS macro#348
iilyak wants to merge 1 commit intoapache:masterfrom
cloudant:set_features

Conversation

@iilyak
Copy link
Copy Markdown
Contributor

@iilyak iilyak commented Sep 25, 2015

We define HAS_CALLBACKS feature macro in order to make dialyzer
happy. OTP releases prior to 15 require defining behaviour_info instead
of a callback attribute. New macro could be used to make conditional
compilation as follows:

    -ifdef(HAS_CALLBACKS).
        -callback app() -> atom().
    -else.
        -export([behaviour_info/1]).
        behaviour_info(callbacks) ->
            [
                {app, 0}
            ];
        behaviour_info(_) ->
            undefined.
    -endif.

The same approach could be used to eliminate the cost of erlang:function_exported in couch_crypto.erl.

@kxepal
Copy link
Copy Markdown
Member

kxepal commented Sep 25, 2015

Good idea, but patch doesn't applied on current master.

@iilyak
Copy link
Copy Markdown
Contributor Author

iilyak commented Sep 25, 2015

I'll do rebase.

We define HAS_CALLBACKS feature macro in order to make dialyzer
happy. OTP releases prior to 15 require defining behaviour_info instead
of callback attribute. New macro could be used to make conditional
compilation as follows:

    -ifdef(HAS_CALLBACKS).
        -callback app() -> atom().
    -else.
        -export([behaviour_info/1]).
        behaviour_info(callbacks) ->
            [
                {app, 0}
            ];
        behaviour_info(_) ->
            undefined.
    -endif.
@iilyak
Copy link
Copy Markdown
Contributor Author

iilyak commented Sep 25, 2015

rebased

@kxepal
Copy link
Copy Markdown
Member

kxepal commented Sep 25, 2015

Thanks! LGFM.

On second thought, we plan to drop R14 support after 2.0 release. And since we don't support R15 even now, do we need this callback workaround in mid/long term?

cc @rnewson

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this could be presented more simply;

OtpVersion = erlang:system_info(compat_rel),
OtpRelease = erlang:system_info(otp_release),
HasCallbacks = OTP_VSN > 14,
Defines = [
    {d, 'OTP_VSN', OtpVersion},
    {d, 'OTP_RELEASE', OtpRelease},
    {d, 'HAS_CALLBACKS', HasCallbacks}
 ],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess @iilyak tried to have more generic solution, but yours simpler and clearer to read. Nice!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hm, the HAS_CALLBACKS one needs more complexity as you say, it needs to not be defined at all in the 'false' case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

further hm, how would we use ?OTP_RELEASE in code? We couldn't use a macro conditional on its value in couch_crypto as far as I can see.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In guards?

md5(Data) when ?OTP_RELEASE >= "17"; ?OTP_RELEASE >= "R15B02" ->
    crypto:hash(md5, Data);
md5(Data) ->
    crypto:md5(Data).

Updated version checks (forgot when crypto deprecation were added).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In conclusion, I think we need more feature flags (HAS_CRYPTO_HASH) and the version ones aren't useful for conditional compilation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Idea with guards will emit another warning about unreachable clause. So it seems the only way is indeed to stay with feature flags.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would call it HAS_CRYPTO_V2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't hardcoded defines as [{d, Key, Value}] is to be able to define some macro from the content of config.erl. But I didn't do a second step to actually use config.erl. I'll update the PR with @rnewson's solution.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given we're supporting R16|17|18 and all the features we might test for are always present, do we need to do any of this right now?

@rnewson
Copy link
Copy Markdown
Member

rnewson commented Sep 29, 2015

I'd like to to see this simplified, it's a good idea and I'd like to simplify couch_crypto when it lands.

@kxepal
Copy link
Copy Markdown
Member

kxepal commented Sep 29, 2015

Good! +1 then when it get simplified.

@iilyak
Copy link
Copy Markdown
Contributor Author

iilyak commented Sep 29, 2015

Should I add HAS_CRYPTO_HASH or HAS_CRYPTO_V2 (let me know which one) in this PR?

@rnewson
Copy link
Copy Markdown
Member

rnewson commented Sep 29, 2015

Since we decided to make R16B03 our minimum, there's no need for HAS_CRYPTO_HASH or HAS_CALLBACKS, we know the features exist.

@iilyak
Copy link
Copy Markdown
Contributor Author

iilyak commented Sep 30, 2015

Closing since we don't need it at the moment.

@iilyak iilyak closed this Sep 30, 2015
@davisp davisp deleted the set_features branch October 22, 2015 06:36
janl pushed a commit that referenced this pull request Jan 5, 2020
nickva pushed a commit to nickva/couchdb that referenced this pull request Sep 7, 2022
…e-impr

Improve clustered purge documentation
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.

4 participants