-
Notifications
You must be signed in to change notification settings - Fork 1k
replace temporary objects with subscript operator #7238
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7238 +/- ##
==========================================
- Coverage 98.79% 98.79% -0.01%
==========================================
Files 81 81
Lines 15254 15247 -7
==========================================
- Hits 15070 15063 -7
Misses 184 184 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No obvious timing issues in HEAD=replaceWithSubscript Generated via commit fbcf862 Download link for the artifact containing the test results: ↓ atime-results.zip
|
anyNA |= elem==NA_INTEGER; | ||
anyLess |= elem<last; | ||
last = elem; | ||
if (idxp[i]<=0 && idxp[i]!=NA_INTEGER) return "Internal inefficiency: idx contains negatives or zeros. Should have been dealt with earlier."; // e.g. test 762 (TODO-fix) |
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.
this one in particular looks useful as a local variable, rather than reading idxp[i]
6 more times.
Is there an advantage of idxp[i]
over just adding const
here?
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.
idxp
is already a pointer to const, so idxp[i]
would likely be held in a register.
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.
"Likely" may not be best reason for change that doesn't really do much. If it is more readable is also questionable... x[y[i]] IMO is less readable than x[yi]
I'm not sure about this, could you elaborate? Any external reference you can point to? |
It's more obvious where the data comes from, instead of adding a temporary object which adds another level of unnecessary abstraction. |
Temporary objects are unnecessary, especially when derived from a const pointer.
I think this creates an improvement in readability, as the code becomes more self documenting.
I would also add that potentially we should make the pointer
idxp
a restrict qualification (if safe) and a more descript name.I don't know exactly what it's for tho, so I'm useless there.