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 full include paths in time/internal/cctz #1003

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Aug 17, 2021

This makes the coding style of time/internal/cctz align with other files, and without using full paths in includes it will be harder for other projects to build together with abseil-cpp.

@google-cla google-cla bot added the cla: yes label Aug 17, 2021
@derekmauro
Copy link
Member

without using full paths in includes it will be harder for other projects to build together with abseil-cpp.

Can you explain further? Is this causing a problem?

@zcbenz
Copy link
Contributor Author

zcbenz commented Aug 18, 2021

I'm merging all abseil's .cc code into one file for compilation and distribution (which is similar to Chromium's jumbo build (which had been removed)), so searching headers from the source code file's directory does not work.

I know this is not something that abseil means to support, but it does reduce compilation time to 1/10 for me :). Also this change makes the code complies to Google C++ Style Guide so I think it might be good to upstream:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

All of a project's header files should be listed as descendants of the project's source directory

@derekmauro
Copy link
Member

The CCTZ code is pulled into Abseil from https://github.com/google/cctz. We have a script that edits the code to make it work in Abseil. I'd have to go modify the script to "import" this pull request. I understand why you want this, but I have some work to do first. It's not high priority for me at this point, but I may get to it soon.

@zcbenz
Copy link
Contributor Author

zcbenz commented Aug 19, 2021

That makes sense, thanks for taking care of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants