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

internal/refcount.h: allow non-atomic build #8479

Closed
wants to merge 2 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Mar 14, 2019

Configure with -DOPENSSL_DEV_NO_ATOMICS and you get refcount without
atomics. This is intended for internal development only, to check the
refcounting is properly coded. It should never become a configuration
option, hence the name of the macro.

@levitte levitte added the branch: master Merge to master branch label Mar 14, 2019
@mspncp
Copy link
Contributor

mspncp commented Mar 14, 2019

This is intended for internal development only

Would it make sense to give the define a less official name then? Say, OPENSSL_INTERNAL_NO_ATOMICS (or whatever you prefer)?

@mspncp
Copy link
Contributor

mspncp commented Mar 14, 2019

Or use OPENSSL_DEV_NO_ATOMICS.

@levitte
Copy link
Member Author

levitte commented Mar 14, 2019

Or use OPENSSL_DEV_NO_ATOMICS.

I like that one. Taken!

Configure with -DOPENSSL_DEV_NO_ATOMICS and you get refcount without
atomics.  This is intended for internal development only, to check the
refcounting is properly coded.  It should never become a configuration
option, hence the name of the macro.
mspncp
mspncp previously approved these changes Mar 14, 2019
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

LGTM


# else
# ifndef HAVE_ATOMICS
Copy link
Contributor

Choose a reason for hiding this comment

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

The most tricky part was to understand this change here, because it requires looking at all the entire ifdef-spagetti. But I think now that it's ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a bit of commentary, I hope that helps

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup!

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW and FYI, just in case you ever had the same problem:

I found it hard to follow the preprocessor indentation using Emacs' incremental search, because by default it treats a single literal space character as 'any whitespace'. But I found a way to change that:

https://emacs.stackexchange.com/questions/2819/match-two-spaces-with-incremental-search

@levitte levitte dismissed mspncp’s stale review March 14, 2019 10:59

Commentary added

Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

I overlooked that you had dismissed my review. Reconfirmed.

(BTW: there is a new icon 'Re-request review' at the top right in the Reviewers list. I assume this will trigger a note.)

@levitte
Copy link
Member Author

levitte commented Mar 14, 2019

(BTW: there is a new icon 'Re-request review' at the top right in the Reviewers list. I assume this will trigger a note.)

Ah! Thanks

@levitte
Copy link
Member Author

levitte commented Mar 14, 2019

Merged.

503d474 internal/refcount.h: allow non-atomic build

@levitte levitte closed this Mar 14, 2019
levitte added a commit that referenced this pull request Mar 14, 2019
Configure with -DOPENSSL_DEV_NO_ATOMICS and you get refcount without
atomics.  This is intended for internal development only, to check the
refcounting is properly coded.  It should never become a configuration
option, hence the name of the macro.

Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #8479)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants