[rpc] remove default constructor in futureInfo #30197
Closed
Conversation
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
This default constructor was added because std::map's operator[] requires a default constructor. However, instead of using operator[], we can use emplace and remove the constructor, to ensure that the FutureInfo struct doesnt get constructed with garbage values. Differential Revision: [D18627675](https://our.internmc.facebook.com/intern/diff/D18627675/) [ghstack-poisoned]
rohan-varma
requested review from
mrshenli,
pritamdamania87 and
zhaojuanmao
as code owners
rohan-varma
added a commit
that referenced
this pull request
Nov 20, 2019
This default constructor was added because std::map's operator[] requires a default constructor. However, instead of using operator[], we can use emplace and remove the constructor, to ensure that the FutureInfo struct doesnt get constructed with garbage values. Differential Revision: [D18627675](https://our.internmc.facebook.com/intern/diff/D18627675/) ghstack-source-id: 94313674 Pull Request resolved: #30197
xush6528
reviewed
Nov 21, 2019
| startTime_(startTime), | ||
| dstRank_(dstRank), | ||
| timeout_(timeout) {} | ||
| FutureInfo() {} |
Contributor
|
@rohan-varma I just caught up diff review about future time out PR in #29601, I think we do not need startTime_ and timeout_, endTime_ should be sufficient. |
Contributor
|
Thanks, @rohan-varma. This makes it just a bit more robust for future changes. |
Contributor
|
I am doing it in #30342. |
Addresses @pietern's comment on #29601. This default constructor was added because `std::map`'s `operator[]` requires a default constructor. However, instead of using `operator[]`, we can use `emplace` and remove the constructor, to ensure that the `FutureInfo` struct doesn't get constructed with garbage values. Differential Revision: [D18627675](https://our.internmc.facebook.com/intern/diff/D18627675/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
Dec 3, 2019
Pull Request resolved: #30197 This default constructor was added because std::map's operator[] requires a default constructor. However, instead of using operator[], we can use emplace and remove the constructor, to ensure that the FutureInfo struct doesnt get constructed with garbage values. ghstack-source-id: 94802453 Differential Revision: [D18627675](https://our.internmc.facebook.com/intern/diff/D18627675/)
wuhuikx
pushed a commit
to wuhuikx/pytorch
that referenced
this pull request
Jan 30, 2020
Summary: Pull Request resolved: pytorch#30197 This default constructor was added because std::map's operator[] requires a default constructor. However, instead of using operator[], we can use emplace and remove the constructor, to ensure that the FutureInfo struct doesnt get constructed with garbage values. ghstack-source-id: 94802453 Test Plan: Unit tests pass. Differential Revision: D18627675 fbshipit-source-id: c4cb000e60081478c0fd7308e17103ebbc4dc554
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
Addresses @pietern's comment on #29601. This default constructor was added because
std::map'soperator[]requires a default constructor. However, instead of usingoperator[], we canuse
emplaceand remove the constructor, to ensure that theFutureInfostructdoesn't get constructed with garbage values.
Differential Revision: D18627675