-
Notifications
You must be signed in to change notification settings - Fork 455
CDRIVER-4215 use legacy atomic built-ins on gcc (>= 4.1, < 4.9) #889
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes LGTM. The atomic test cases are thorough enough that they will recognize any obviously-wrong changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. Thank you for investigating. I have comments about macros since they are in public headers, but no concerns about correctness.
b8ce3a9
to
7df151b
Compare
Full patch build with these changes: https://spruce.mongodb.com/version/618d4fc82fbabe51bb1beb05/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC |
The last changes that I pushed constitute two things:
The full patch build that I submitted shows that on RHEL 6 and 7 the compile tasks and now some of the tests pass. There are still numerous test tasks that are failing, but those failures are unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Assert (got, ==, 9); \ | ||
Assert (value, ==, 9); \ | ||
/* Compare-exchange-weak succeed: */ \ | ||
/* 'weak' may fail spuriously, so it must *eventually* succeed */ \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't argue with that haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but one comment about naming and clarity on a macro
@vector-of-bool Does that comment work for you? |
Yes, that comment is helpful |
8533dd9
to
066e059
Compare
This work is not yet complete. However, given the scope of the change just in getting the compile to succeed on RHEL 6, it seemed prudent to push my first set of changes as a WIP PR.
Initial patch build: https://spruce.mongodb.com/version/61874ce932f4177a1ad61efb/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC
@vector-of-bool In the patch build, you will want to focus on the compile task in the
rhel62
variant. The test tasks are still failing, but for unrelated reasons. At this point I am specifically interested to know if you find any errors or deficiencies in my initial changes.After I get the compile working on RHEL 7 I will take a look at all the warnings that are still being produced.