This repository has been archived by the owner on Oct 15, 2020. It is now read-only.
src: fix warnings in aliased_buffer #503
Closed
Conversation
This file contains 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
sethbrenith
reviewed
Mar 26, 2018
src/aliased_buffer.h
Outdated
| @@ -129,7 +129,7 @@ class AliasedBuffer { | |||
|
|
|||
| template <typename T> | |||
| inline Reference& operator=(const T& val) { | |||
| aliased_buffer_->SetValue(index_, val); | |||
| aliased_buffer_->SetValue(index_, static_cast<NativeT>(val)); | |||
There was a problem hiding this comment.
I agree that this does the same as the original code, but I'd prefer a solution where these operators aren't templates and the caller has to think about whether casting is valid. Otherwise we hide all of the danger inside this function instead of getting useful compiler warnings elsewhere.
There was a problem hiding this comment.
you'd still get compiler warnings w/ static_cast right? I don't remember why these functions were templated instead of taking a NativeT as a param.
There was a problem hiding this comment.
No, the explicit cast tells the compiler that you've carefully thought about what happens here and are okay with a possibly lossy conversion.
In reply to: 177183603 [](ancestors = 177183603)
There was a problem hiding this comment.
I see. So, it would be interesting to see if we could remove the templated functions here & push casts onto the caller. I don't recall why I did it this way previously.
sethbrenith
reviewed
Mar 26, 2018
src/aliased_buffer.h
Outdated
| @@ -144,7 +144,7 @@ class AliasedBuffer { | |||
|
|
|||
| template <typename T> | |||
| inline Reference& operator+=(const T& val) { | |||
| const T current = aliased_buffer_->GetValue(index_); | |||
| const NativeT current = aliased_buffer_->GetValue(index_); | |||
There was a problem hiding this comment.
I don't think this is exactly equivalent to the existing code; it's actually a little better. If NativeT is wider than T, then it used to cast the existing value to T, perform the addition in T-space, and then cast back to NativeT. Now it should perform the addition as whichever is wider of T and NativeT since you're adding one of each, which could plausibly extend beyond the bounds of T.
|
lgtm |
mike-kaufman
approved these changes
Mar 26, 2018
sethbrenith
reviewed
Mar 26, 2018
src/aliased_buffer.h
Outdated
| inline Reference& operator=(const T& val) { | ||
| aliased_buffer_->SetValue(index_, val); | ||
| inline Reference& operator=(const NativeT& val) { | ||
| aliased_buffer_->SetValue(index_, static_cast<NativeT>(val)); |
There was a problem hiding this comment.
static_cast [](start = 40, length = 11)
static_cast from NativeT to NativeT is no longer needed
|
Looks like there's a CI failure on macOS: I'll have to investigate that before taking this upstream. |
* Unary minus usages on unsigned type * Implicit casts to/from input parameters
|
Opened upstream PR: nodejs/node#19665 |
|
Landed upstream PR nodejs/node#19665 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Just looking for feedback from this group first, then I'll open a PR upstream. I saw these warnings while working on another issue and I believe these fixes to be equivalent to the implicit conversions that were happening before.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes