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

Use local macro names to avoid conflicts with gflags. #96

Merged
merged 2 commits into from
Jun 11, 2020

Conversation

gregcoombe
Copy link
Contributor

The gflags macro names (DEFINE_) are also used by glog. If you have a setup that uses glog but not gflags, the glog/logging.h header will actually undefine these macros. This causes a very difficult-to-find error in S2. This PR changes the names of these macros to a library-specific name to avoid this issue. It follows the pattern of the logging macros.

src/s2/base/commandlineflags.h Outdated Show resolved Hide resolved
@jmr
Copy link
Member

jmr commented May 8, 2020

It seems like this is a glog bug, but we probably just have to work around it. Can you file a bug / send a PR there?

@gregcoombe
Copy link
Contributor Author

It seems like this is a glog bug, but we probably just have to work around it. Can you file a bug / send a PR there?

So I'm not sure that this is really a glog bug, or that they would be able to resolve it. It's more of an issue with how the different libraries are used within my system, which interacts badly with the fact that S2 uses the same names (in a global namespace) as gflags for macros.
The reason that I thought that this would be a good fix is that using library-specific macros is a better practice overall (S2 already does this for glog) and it solves my immediate problem. So I don't really think of this as a work-around! I think of it as a tiny step forward in the design of the library.

@jmr jmr merged commit 973f0e4 into google:master Jun 11, 2020
gregcoombe added a commit to gregcoombe/s2geometry that referenced this pull request Jun 15, 2020
Use `S2_DEFINE_int32` etc instead of `DEFINE_int32`.
gregcoombe added a commit to gregcoombe/s2geometry that referenced this pull request Aug 14, 2020
Use `S2_DEFINE_int32` etc instead of `DEFINE_int32`.
@jievince
Copy link

I am using V9.0 and this problem also took me a long time to debug

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.

3 participants