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

Add S2_USE_SYSTEM_INCLUDES cmake options to avoid s2 compile time warnings… #300

Merged
merged 2 commits into from
Jan 12, 2023

Conversation

MBkkt
Copy link
Contributor

@MBkkt MBkkt commented Jan 9, 2023

… from s2 headers

@jmr jmr mentioned this pull request Jan 9, 2023
@jmr
Copy link
Member

jmr commented Jan 9, 2023

What warnings are you seeing? Maybe we can just fix them.

@MBkkt
Copy link
Contributor Author

MBkkt commented Jan 9, 2023

@jmr

What warnings are you seeing? Maybe we can just fix them.

I think you're fixing part of them in update from Google PR.

But in general I don't think it's convenient for us as users to create PR for every warning (especially for msvc, array[0], undef, etc):

Please check commit

arangodb/arangodb@47b0f23

@jmr
Copy link
Member

jmr commented Jan 9, 2023

Please check commit arangodb/arangodb@47b0f23

How do I see the warnings?

Can you paste the output of what you see without SYSTEM, or something similar?

@MBkkt
Copy link
Contributor Author

MBkkt commented Jan 9, 2023

@jmr

You can see what it cause:

Zero sized array
https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-levels-2-and-4-c4200?view=msvc-170

define windows related macros before undef they

not cast to void* in memset trivial struct

A lot of int/size_t

Can you paste the output of what you see without SYSTEM, or something similar?

I don't have windows local, so I need run it in our CI.

And we don't compile s2 with warnings, so issue with werror fails because of s2, commonly depends on which headers I include.
I don't include all, so if we fix some warnings now, it's not guarantee that in future I didn't face with same issue but in other header.

@MBkkt
Copy link
Contributor Author

MBkkt commented Jan 9, 2023

In general It's not really difficult to me have such patch local.

I just thought it can be useful for other users.

If you think it's really unnecessary (why?) I can close it.

If you want fix warnings, I think better to add it to S2 tests and CI

@jmr
Copy link
Member

jmr commented Jan 10, 2023

abseil-cpp does it, so that's good enough for me.

Can you make it an option?

https://github.com/abseil/abseil-cpp/blob/2ed6963f2b67a68d94303cafb571d3d039a49f9a/CMakeLists.txt#L75

@MBkkt
Copy link
Contributor Author

MBkkt commented Jan 10, 2023

Ok!

@MBkkt
Copy link
Contributor Author

MBkkt commented Jan 10, 2023

@jmr Done

@MBkkt MBkkt changed the title Add S2_SYSTEM_HEADERS cmake options to avoid s2 compile time warnings… Add S2_USE_SYSTEM_INCLUDES cmake options to avoid s2 compile time warnings… Jan 10, 2023
@jmr jmr merged commit efb4eb8 into google:master Jan 12, 2023
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.

None yet

2 participants