Push anonymous namespace into codegen, not template#49498
Closed
ezyang wants to merge 6 commits intogh/ezyang/897/basefrom
Closed
Push anonymous namespace into codegen, not template#49498ezyang wants to merge 6 commits intogh/ezyang/897/basefrom
ezyang wants to merge 6 commits intogh/ezyang/897/basefrom
Conversation
In the near future, I want to code generate some functions that are visible externally to this compilation unit. I cannot easily do this if all the codegen code is wrapped in a global anonymous namespace, so push the namespace in. Registration has to stay in an anonymous namespace to avoid name conflicts. This could also have been solved by making the wrapper functions have more unique names but I didn't do this in the end. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
This was referenced Dec 16, 2020
Contributor
💊 CI failures summary and remediationsAs of commit 23d2a76 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. This comment has been revised 50 times. |
ezyang
added a commit
that referenced
this pull request
Dec 16, 2020
In the near future, I want to code generate some functions that are visible externally to this compilation unit. I cannot easily do this if all the codegen code is wrapped in a global anonymous namespace, so push the namespace in. Registration has to stay in an anonymous namespace to avoid name conflicts. This could also have been solved by making the wrapper functions have more unique names but I didn't do this in the end. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 3254eac Pull Request resolved: #49498
In the near future, I want to code generate some functions that are visible externally to this compilation unit. I cannot easily do this if all the codegen code is wrapped in a global anonymous namespace, so push the namespace in. Registration has to stay in an anonymous namespace to avoid name conflicts. This could also have been solved by making the wrapper functions have more unique names but I didn't do this in the end. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
In the near future, I want to code generate some functions that are visible externally to this compilation unit. I cannot easily do this if all the codegen code is wrapped in a global anonymous namespace, so push the namespace in. Registration has to stay in an anonymous namespace to avoid name conflicts. This could also have been solved by making the wrapper functions have more unique names but I didn't do this in the end. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
In the near future, I want to code generate some functions that are visible externally to this compilation unit. I cannot easily do this if all the codegen code is wrapped in a global anonymous namespace, so push the namespace in. Registration has to stay in an anonymous namespace to avoid name conflicts. This could also have been solved by making the wrapper functions have more unique names but I didn't do this in the end. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D25616104](https://our.internmc.facebook.com/intern/diff/D25616104) [ghstack-poisoned]
In the near future, I want to code generate some functions that are visible externally to this compilation unit. I cannot easily do this if all the codegen code is wrapped in a global anonymous namespace, so push the namespace in. Registration has to stay in an anonymous namespace to avoid name conflicts. This could also have been solved by making the wrapper functions have more unique names but I didn't do this in the end. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D25616104](https://our.internmc.facebook.com/intern/diff/D25616104) [ghstack-poisoned]
In the near future, I want to code generate some functions that are visible externally to this compilation unit. I cannot easily do this if all the codegen code is wrapped in a global anonymous namespace, so push the namespace in. Registration has to stay in an anonymous namespace to avoid name conflicts. This could also have been solved by making the wrapper functions have more unique names but I didn't do this in the end. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D25616104](https://our.internmc.facebook.com/intern/diff/D25616104) [ghstack-poisoned]
bdhirsh
reviewed
Jan 5, 2021
|
|
||
| namespace { | ||
|
|
||
| ${dispatch_definitions} |
Collaborator
There was a problem hiding this comment.
Do you plan on potentially generating functions with different namespaces in the same Register{DispatchKey}.cpp file? If it'll always be uniform per file, it might be cleaner to code-template the name of the namespace directly. So you can look at something like RegisterCPU.cpp and easily say "oh yeah these functions are all in the same namespace"
Contributor
Author
There was a problem hiding this comment.
No, they'll either be anonymous, or in the cpu namespace (for example).
smessmer
approved these changes
Jan 5, 2021
Contributor
smessmer
left a comment
There was a problem hiding this comment.
What's your reason for putting it in codegen? As long as it unconditionally applies to the whole file, it'd be easier to have it in the template.
bhosmer
approved these changes
Jan 5, 2021
bhosmer
left a comment
There was a problem hiding this comment.
Makes sense. As an aside, making wrapper names more unique would've been a nice improvement in greppability of the generated code, so I'm kind of sad you didn't wind up doing this but I imagine it would've been a substantially bigger undertaking.
Contributor
Author
In the next PR this invariant is violated! |
Contributor
Author
It's not that hard. I'll put it on the cleanup list. |
Contributor
hwangdeyu
pushed a commit
to hwangdeyu/pytorch
that referenced
this pull request
Jan 14, 2021
Summary: Pull Request resolved: pytorch#49498 In the near future, I want to code generate some functions that are visible externally to this compilation unit. I cannot easily do this if all the codegen code is wrapped in a global anonymous namespace, so push the namespace in. Registration has to stay in an anonymous namespace to avoid name conflicts. This could also have been solved by making the wrapper functions have more unique names but I didn't do this in the end. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Test Plan: Imported from OSS Reviewed By: albanD, smessmer Differential Revision: D25616104 Pulled By: ezyang fbshipit-source-id: 323c0dda05a081502aab702f359a08dfac8c41a4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
In the near future, I want to code generate some functions that are
visible externally to this compilation unit. I cannot easily do this
if all the codegen code is wrapped in a global anonymous namespace,
so push the namespace in.
Registration has to stay in an anonymous namespace to avoid name conflicts.
This could also have been solved by making the wrapper functions have
more unique names but I didn't do this in the end.
Signed-off-by: Edward Z. Yang ezyang@fb.com
Differential Revision: D25616104