Skip to content

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Jan 18, 2022

This PR aims to resolve CDRIVER-4237, depended on by PHP-2013.

Four Three types of warnings are addressed by this PR (as described by GCC 4.8.5):

  • ISO C does not allow extra ; outside of a function [-Wpedantic]
  • passing argument {N} of {func} from incompatible pointer type [enabled by default]
  • pointer targets in passing argument {N} of {func} differ in signedness [-Wpointer-sign]
  • empty macro arguments are undefined in ISO C90 and ISO C++98 [enabled by default]

Followup changes in addition to the current set may need to be added to fully resolve PHPC-2013. These will be added as demanded in response to code review.

@eramongodb
Copy link
Contributor Author

Reverted "fixes" to empty macro arguments that were not sufficiently tested for correctness.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

The changes look simple enough. Is it really that the compilers mishandle typedefs for their intrinsics? That surprises me.

My only question is about formatting: The prior (wonky) formatting came from my clang-format frantically trying to enforce the 80 column limit (which is much too small, imo), so the the prior formatting satisfied the .clang-format in the repository. What version of clang-format are you using? Or did you do some other formatting formatting?

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Thank you for the fix. The current changes LGTM. Please re-request review if there are followup changes.

@eramongodb
Copy link
Contributor Author

What version of clang-format are you using? Or did you do some other formatting formatting?

@vector-of-bool I am using .clang-format as provided by the C driver repository. Normally I use the clang-format executable as provided via VS Code's C/C++ extension. Running the system clang-format executable (clang version 10.0.0-4ubuntu1) from project root with the command clang-format --style=file -i src/libbson/src/bson/bson-atomic.h yields the same result.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

This patch fixed PHP builds on zSeries. PPC is still failing for unrelated reasons (tasks), but I've noted the successful PPC builds for libmongoc (tasks) so I have no objections to merging.

@eramongodb eramongodb merged commit 7a35cf8 into mongodb:master Jan 19, 2022
@eramongodb eramongodb deleted the cdriver-4237 branch January 19, 2022 19:07
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