Fix C++ warnings - #1843
Conversation
a04c547 to
254d71c
Compare
4679975 to
0155217
Compare
|
Any updates? |
3d676a5 to
766edd9
Compare
|
It's not every day you discover MSVC is the one with a compilation bug, considering it is why only one test failed due to a refactoring despite other compilers generating identical code |
|
All ready! |
|
A link to what I think about the warning fixes: microsoft/STL#3079 (comment) |
|
A lot of these changes are modernizations. Every single refactoring has been thoroughly tested and there is no behavioral change outside of some functions being changed to an equivalent form. |
|
Can you please stop rebasing and pushing? Microsoft usually do squash merging anyway so your rebases are moot. |
Check for exactly 1 before using the singular noun, rather than check for greater than 1, just in case somehow the variable being evaluated ends up as 0 In addition, should this code ever be refactored in such a way that the check to 0 is changed, this will ensure the end result does not change. In addition, it is naturally immediately apparent to the developers what is going on, potentially enhancing maintainability. Additionally, make bitwise operations more obvious in their intentions. Fix unused parameters in functions by using [[maybe_unused]]
|
Thanks for all the work you've put into this PR! Unfortunately, this change is too big for us to review. The feedback you received in the microsoft/STL repo (microsoft/STL#3079 (comment)) is good advice here in microsoft/calculator too. We would be happy to take many of these changes, but please break them into smaller PRs with good descriptions. |
|
Also consider forgetting how to rebase and force push as it's only ever needed ONLY if you accidentally push credentials onto github like API tokens (to quickly remove them) but then again, they will instantly become invalidated if they are github ones. That should be the ONLY reason to force push, other than for merge conflicts (unless Microsoft says otherwise which they keep telling you to not do it). |
Fixes