Skip to content

Remove using namespace from header files #23575

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
Merged
merged 1 commit into from
Sep 6, 2024
Merged

Remove using namespace from header files #23575

merged 1 commit into from
Sep 6, 2024

Conversation

r-barnes
Copy link
Contributor

@r-barnes r-barnes commented Sep 3, 2024 •
edited by amitkdutta
Loading

Removes using namespace from header files to align with Velox C++ codestyle guidelines (link).

@r-barnes r-barnes requested a review from a team as a code owner September 3, 2024 15:00
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 3, 2024
@r-barnes @facebook-github-bot
Enable -Wheader-hygiene for github/PACKAGE +1 (prestodb#23575)
3c82607 
Summary:
Pull Request resolved: prestodb#23575

Per title

#buildmore - Be thorough

#buildsonlynotests - No runtime effects!

 - If you approve of this diff, please use the "Accept & Ship" button

no-ig-test - Doesn't require Instagram testing.

(1 file modified.)

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 3, 2024
@r-barnes @facebook-github-bot
Enable -Wheader-hygiene for github/PACKAGE +1 (prestodb#23575)
7f1f97c 
Summary:
Pull Request resolved: prestodb#23575

Per title

#buildmore - Be thorough

#buildsonlynotests - No runtime effects!

 - If you approve of this diff, please use the "Accept & Ship" button

no-ig-test - Doesn't require Instagram testing.

(1 file modified.)

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 3, 2024
@r-barnes @facebook-github-bot
Enable -Wheader-hygiene for github/PACKAGE +1 (prestodb#23575)
d974d51 
Summary:
Pull Request resolved: prestodb#23575

Per title

#buildmore - Be thorough

#buildsonlynotests - No runtime effects!

 - If you approve of this diff, please use the "Accept & Ship" button

no-ig-test - Doesn't require Instagram testing.

(1 file modified.)

Reviewed By: dmm-fb

Differential Revision: D56538909
r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 3, 2024
@r-barnes @facebook-github-bot
Enable -Wheader-hygiene for github/PACKAGE +1 (prestodb#23575)
d6bf027 
Summary:
Pull Request resolved: prestodb#23575

Per title

#buildmore - Be thorough

#buildsonlynotests - No runtime effects!

 - If you approve of this diff, please use the "Accept & Ship" button

no-ig-test - Doesn't require Instagram testing.

(1 file modified.)

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

1 similar comment
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 3, 2024
@r-barnes @facebook-github-bot
Enable -Wheader-hygiene for github/PACKAGE +1 (prestodb#23575)
d983f85 
Summary:
Pull Request resolved: prestodb#23575

Per title

#buildmore - Be thorough

#buildsonlynotests - No runtime effects!

 - If you approve of this diff, please use the "Accept & Ship" button

no-ig-test - Doesn't require Instagram testing.

(1 file modified.)

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 3, 2024
@r-barnes @facebook-github-bot
Enable -Wheader-hygiene for github/PACKAGE +1 (prestodb#23575)
1f55b52 
Summary:
Pull Request resolved: prestodb#23575

Per title

#buildmore - Be thorough

#buildsonlynotests - No runtime effects!

 - If you approve of this diff, please use the "Accept & Ship" button

no-ig-test - Doesn't require Instagram testing.

(1 file modified.)

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 3, 2024
@r-barnes @facebook-github-bot
Enable -Wheader-hygiene for github/PACKAGE +1 (prestodb#23575)
735ebcd 
Summary:
Pull Request resolved: prestodb#23575

Per title

#buildmore - Be thorough

#buildsonlynotests - No runtime effects!

 - If you approve of this diff, please use the "Accept & Ship" button

no-ig-test - Doesn't require Instagram testing.

(1 file modified.)

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 4, 2024
@r-barnes @facebook-github-bot
Enable -Wheader-hygiene for github/PACKAGE +1 (prestodb#23575)
d253a7b 
Summary:
Pull Request resolved: prestodb#23575

Per title

#buildmore - Be thorough

#buildsonlynotests - No runtime effects!

 - If you approve of this diff, please use the "Accept & Ship" button

no-ig-test - Doesn't require Instagram testing.

(1 file modified.)

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 4, 2024
@r-barnes @facebook-github-bot
Enable -Wheader-hygiene for github/PACKAGE +1 (prestodb#23575)
b217e7d 
Summary:
Pull Request resolved: prestodb#23575

Per title

#buildmore - Be thorough

#buildsonlynotests - No runtime effects!

 - If you approve of this diff, please use the "Accept & Ship" button

no-ig-test - Doesn't require Instagram testing.

(1 file modified.)

Reviewed By: dmm-fb

Differential Revision: D56538909
@rschlussel
Copy link
Contributor

I think -Wheader-hygiene is an internal concept?

Can you change the PR title/description and commit message title/description to be clearer about what this is doing and why. It sounds like what this is doing is removing "using namespace xxx" from header files, as is aligned with our c++ codestyle guidelines, which follows the velox style guide https://github.com/facebookincubator/velox/blob/main/CODING_STYLE.md#namespaces.

@r-barnes r-barnes changed the title Enable -Wheader-hygiene for github/PACKAGE +1 Remove using namespace from header files Sep 4, 2024
@r-barnes
Copy link
Contributor Author

r-barnes commented Sep 4, 2024

@rschlussel - I've made the requested updates.

@rschlussel
Copy link
Contributor

Can you update the commit message as well (same description/title as the pr description)?

@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

rschlussel
rschlussel approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

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

thanks! I need someone from team-velox to approve @amitkdutta

@r-barnes
Copy link
Contributor Author

r-barnes commented Sep 5, 2024

@rschlussel - done

@rschlussel
Copy link
Contributor

@r-barnes I think you need to split the presto part of this change out from the other part for your diff.

@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 5, 2024
@r-barnes @facebook-github-bot
Remove using namespace from header files (prestodb#23575)
54ffe96 
Summary:
Pull Request resolved: prestodb#23575

Removes using namespace from header files to align with Velox C++ codestyle guidelines

Reviewed By: dmm-fb

Differential Revision: D56538909
r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 5, 2024
@r-barnes @facebook-github-bot
Remove using namespace from header files (prestodb#23575)
1572211 
Summary:
Pull Request resolved: prestodb#23575

Removes using namespace from header files to align with Velox C++ codestyle guidelines

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

r-barnes added a commit to r-barnes/presto that referenced this pull request Sep 5, 2024
@r-barnes @facebook-github-bot
Remove using namespace from header files (prestodb#23575)
2d13dde 
Summary:
Pull Request resolved: prestodb#23575

Removes using namespace from header files to align with Velox C++ codestyle guidelines

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

@r-barnes @facebook-github-bot
Remove using namespace from header files (prestodb#23575)
20f5423 
Summary:
Pull Request resolved: prestodb#23575

Removes using namespace from header files to align with Velox C++ codestyle guidelines

Reviewed By: dmm-fb

Differential Revision: D56538909
@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D56538909

aditi-pandit
aditi-pandit approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @r-barnes.

@r-barnes
Copy link
Contributor Author

r-barnes commented Sep 6, 2024

@aditi-pandit - How do I merge?

@rschlussel rschlussel merged commit f393c60 into prestodb:master Sep 6, 2024
60 of 61 checks passed
AbhijitKulkarni1 pushed a commit to intel-staging/presto that referenced this pull request Oct 1, 2024
@r-barnes @AbhijitKulkarni1
Remove using namespace from header files (prestodb#23575)
8e8dec5 
Summary:
Pull Request resolved: prestodb#23575

Removes using namespace from header files to align with Velox C++ codestyle guidelines

Reviewed By: dmm-fb

Differential Revision: D56538909
@r-barnes r-barnes deleted the export-D56538909 branch October 14, 2024 21:42
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
Merged
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rschlussel rschlussel rschlussel approved these changes

@aditi-pandit aditi-pandit aditi-pandit approved these changes

Assignees
No one assigned
Labels
fb-exported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@r-barnes @facebook-github-bot @rschlussel @aditi-pandit