-
Notifications
You must be signed in to change notification settings - Fork 122
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.
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
Use a reference to store the LHS of assignments #981
Conversation
clang-tidy review says "All clean, LGTM! 👍" |
8789b9e
to
3404230
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #981 +/- ##
==========================================
- Coverage 93.94% 93.80% -0.15%
==========================================
Files 55 55
Lines 8062 8065 +3
==========================================
- Hits 7574 7565 -9
- Misses 488 500 +12
... and 1 file with indirect coverage changes
|
clang-tidy review says "All clean, LGTM! 👍" |
test/Gradient/Assignments.C
Outdated
//CHECK-NEXT: double &_t1 = (t *= x); | ||
//CHECK-NEXT: _t2 = t; | ||
//CHECK-NEXT: _t1 *= y; | ||
//CHECK-NEXT: _t1 = &(t *= x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that right? Aren't we taking the address of a temporary that gets destroyed at the end of the expression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of t *= x
is a reference to t
. So &(t *= x)
should give the address of t
. Anyway, we used to declare a reference to the same expression and references are pointers under the hood. So this change shouldn't affect the performance or behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, I see. Thanks!
@PetroZarytskyi, #971 is merged. Can you rebase? |
3404230
to
4c53c1f
Compare
clang-tidy review says "All clean, LGTM! 👍" |
4c53c1f
to
cf0984b
Compare
clang-tidy review says "All clean, LGTM! 👍" |
cf0984b
to
d4c0d6c
Compare
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I believe the codecov complaint is because we removed more code and it thinks the coverage is reduced... |
This PR is created to fix #759.