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.

Sign up for GitHub

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

Jump to bottom

feat(common): adds begin/end inline namespace macros #7456

Merged
merged 2 commits into from
Oct 13, 2021
Merged

feat(common): adds begin/end inline namespace macros #7456

merged 2 commits into from
Oct 13, 2021

Conversation

devjgm
Copy link
Contributor

@devjgm devjgm commented Oct 13, 2021 •
edited
Loading

Part of: #5976
Part of: #7453

This change is Reviewable

@devjgm devjgm requested a review from a team as a code owner October 13, 2021 20:23
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 13, 2021
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 739f362422964575c47e3e0c75570b3b3b14999e

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

coryan
coryan approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

I do not think we have every used inline-ns as a component before.
Consider feat(common). Or simply feat:

@devjgm devjgm changed the title feat(inline-ns): adds begin/end inline namespace macros feat(common): adds begin/end inline namespace macros Oct 13, 2021
@devjgm
Copy link
Contributor Author

devjgm commented Oct 13, 2021

I do not think we have every used inline-ns as a component before. Consider feat(common). Or simply feat:

Done.

devbww
devbww reviewed Oct 13, 2021
View reviewed changes
google/cloud/version.h Outdated
Comment on lines 49 to 51
#define GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN \
inline namespace GOOGLE_CLOUD_CPP_NS {
#define GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END }
Copy link
Contributor

Choose a reason for hiding this comment

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

Q. Did you decide against putting the compatibility namespace alias(es) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, just will do those in a separate PR.

google/cloud/version.h Outdated
Comment on lines 49 to 51
#define GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN \
inline namespace GOOGLE_CLOUD_CPP_NS {
#define GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END }
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though the comment is on the macro and not the substituted curly brace, a // namespace GOOGLE_CLOUD_CPP_NS here might make things even clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: If/when you add the compatibility aliases, this will have to change to /* ... */. Maybe it makes sense to do that now?

google/cloud/version.h
* are versioned, i.e., the symbol becomes `google::cloud::vXYZ::Foo`.
*/
#define GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN \
inline namespace GOOGLE_CLOUD_CPP_NS {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we've limited use of GOOGLE_CLOUD_CPP_NS, perhaps it is time to slip INLINE (or something) in there. At the moment it sounds like it refers to google::cloud.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that separately. To avoid breaking users, we'd need to keep the old macro name around for a while at least.

@codecov
Copy link

codecov bot commented Oct 13, 2021 •
edited
Loading

Codecov Report

Merging #7456 (6708a24) into main (8751407) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7456      +/-   ##
==========================================
- Coverage   93.64%   93.64%   -0.01%     
==========================================
  Files        1364     1364              
  Lines      118359   118359              
==========================================
- Hits       110841   110838       -3     
- Misses       7518     7521       +3
Impacted Files Coverage Δ
google/cloud/version.h 100.00% <ø> (ø)
google/cloud/bigtable/internal/common_client.cc 95.71% <0.00%> (-1.43%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 97.75% <0.00%> (-0.25%) ⬇️
google/cloud/completion_queue_test.cc 96.95% <0.00%> (-0.20%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8751407...6708a24. Read the comment docs.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 6708a24563711346b6ad1502403886cbb853ddb2

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

devbww
devbww reviewed Oct 13, 2021
View reviewed changes
cmake/GoogleCloudCppCommon.cmake
Comment on lines +83 to +84
"GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN=inline namespace omit_this_inline_ns {"
"GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END=}"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these would work as (and be better as) EXPAND_AS_DEFINED names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to keep them as they are because this is only temporary and the actual change I want to make in about 2 PRs is to change them to be defined as empty string, which should be done like this.

google/cloud/version.h Outdated
Comment on lines 49 to 51
#define GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN \
inline namespace GOOGLE_CLOUD_CPP_NS {
#define GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: If/when you add the compatibility aliases, this will have to change to /* ... */. Maybe it makes sense to do that now?

@devjgm
Copy link
Contributor Author

devjgm commented Oct 13, 2021

Nit: If/when you add the compatibility aliases, this will have to change to /* ... */. Maybe it makes sense to do that now?

We can talk about that later when adding the compat alias. I'd probably lean toward just removing the comment since the macro name is clearly named and the comment doesn't end up in the expanded code anyway.

@devjgm devjgm merged commit 9142060 into googleapis:main Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devbww devbww devbww approved these changes

@coryan coryan coryan approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devjgm @google-cloud-cpp-bot @coryan @devbww @googlebot