Fix some C++ warnings #3079

AZero13 · 2022-09-05T18:23:40Z

No description provided.

AlexGuteniev

This could be better if one warning type at a time is addressed

stl/src/vector_algorithms.cpp

stl/src/stacktrace.cpp

stl/src/StlCompareStringA.cpp

killerswin2 · 2022-09-05T23:42:02Z

On a more general note of development. You might want to have more descriptive commit messages rather than "Update filename". We know you are updating/changing the file. Commits should have a message that states what you're doing. like "made auto cp , auto *cp to distinguish pointer"

killerswin2 · 2022-09-05T23:43:38Z

Also, you might want to tell us in the pr header why you made these changes or what motivated you do these changes. Just saying fixing some C++ warnings is very generic.

MattStephanson · 2022-09-05T23:55:09Z

In Visual Studio, at least, you can use Ctrl+K, Ctrl+D to format an entire file. It seems like you could save some effort (and CI resources) by first doing this on every file you've modified.

AraHaan

Looks good on files I did not add comments on, on files that I did, there are comments about a few nits I have.

stl/inc/xcharconv_ryu.h

stl/src/_tolower.cpp

stl/src/_toupper.cpp

stl/src/cond.cpp

stl/src/cthread.cpp

stl/src/vector_algorithms.cpp

stl/src/xalloc.cpp

stl/src/xdateord.cpp

tools/validate/validate.cpp

sam20908 · 2022-09-06T15:08:30Z

I think this PR is tackling too much at once, hence very difficult to review as a whole. It would be nice if there was a dedicated PR to tackle certain kinds of warnings at a time.

AraHaan · 2022-09-06T16:46:25Z

While I appreciate the effort in this PR, I think that maintainers or someone else should take over the changes in this PR (and split them into separate PR's that are grouped together each with similar changes).

Because of this I feel that in the current state this PR is in that it is not worth merging as it's just too large to review with a lot of unrelated whitespace changes that make review unrealistically harder to do.

Also the force pushes did not need to be used because squash merging usually is used (I think) anyway.

AraHaan · 2022-09-06T19:50:24Z

After thinking about it more, I fail to see the point of fixing these warnings so that they do not show up when using /Wall, however I personally never use /Wall as I find it is not worth it because usually it emits warnings that are false positives and so I usually use up to /W3 or /W4. In this case if you use /Wall you deserve to get false positive warnings because you asked for all warnings (even ones that can emit false positives).

StephanTLavavej · 2022-09-07T01:08:36Z

Thanks for looking into improving the codebase. However, as other contributors have mentioned, there is way too much happening in this PR, and with no explanation. I've previously explained in #2766 (comment) why "grab bag" PRs with no descriptions are risky and costly to review. (There are also some behavioral changes mixed in here, which are even riskier.) While we're interested in long-term codebase health and merge a significant number of code cleanups over time, we are also quite busy with feature/bugfix work, so it's important for "nice to have" enhancements to be low risk and easy to understand/review.

Additionally, as Casey previously explained in #2083 (comment), force-pushes should be avoided in microsoft/STL PRs - they make incremental reviewing difficult. (In rare situations they can be reasonable, but this is not one of them.)

I note that the STL currently attempts to be /W4 /analyze clean, but does not promise to be /Wall clean - that is tracked by #186 and we might decide that the status quo is reasonable (I've marked it as "decision needed").

Please let us know if you're interested in decomposing this PR into a series of small, focused PRs with explanations, where we can easily decide whether to merge each category of changes. (For example, changing const variables to constexpr is one category - even if a number of files are affected, the changes are easy to review. Changing const in function signatures is a different category of change - e.g. the _Strxfrm() signature change here should not be performed as that function is separately compiled, even though it's usually extern "C".)

AZero13 · 2022-09-07T12:26:45Z

Thanks for looking into improving the codebase. However, as other contributors have mentioned, there is way too much happening in this PR, and with no explanation. I've previously explained in #2766 (comment) why "grab bag" PRs with no descriptions are risky and costly to review. (There are also some behavioral changes mixed in here, which are even riskier.) While we're interested in long-term codebase health and merge a significant number of code cleanups over time, we are also quite busy with feature/bugfix work, so it's important for "nice to have" enhancements to be low risk and easy to understand/review.

Additionally, as Casey previously explained in #2083 (comment), force-pushes should be avoided in microsoft/STL PRs - they make incremental reviewing difficult. (In rare situations they can be reasonable, but this is not one of them.)

I note that the STL currently attempts to be /W4 /analyze clean, but does not promise to be /Wall clean - that is tracked by #186 and we might decide that the status quo is reasonable (I've marked it as "decision needed").

Please let us know if you're interested in decomposing this PR into a series of small, focused PRs with explanations, where we can easily decide whether to merge each category of changes. (For example, changing const variables to constexpr is one category - even if a number of files are affected, the changes are easy to review. Changing const in function signatures is a different category of change - e.g. the _Strxfrm() signature change here should not be performed as that function is separately compiled, even though it's usually extern "C".)

I am very interested

Remove unneeded casts, as well as perform simplifications that could improve performance.

tylerbrawl

I agree with the sentiment that changes like this are too broad to perform in one commit like this. With that said, I've looked over the changes made and offered some suggestions. Here are some key notes:

I like the additions of constexpr where they can be used. Those should go into their own PR, in my opinion.
A number of changes here are either redundant or appear to conflict with the style conventions used in the rest of the codebase. These will need to be resolved.
At least one change seems to affect the logic of the code (specifically, the change to remove --qh in the _MP_Rem() function found in multprec.cpp). If you removed this because it was a bug, then that should probably go in a separate PR.

stl/inc/__msvc_int128.hpp

tylerbrawl · 2022-09-07T11:56:32Z

stl/inc/xcharconv_ryu.h

@@ -703,7 +703,7 @@ _NODISCARD pair<_CharT*, errc> __d2fixed_buffered_n(_CharT* _First, _CharT* cons

 _NODISCARD inline to_chars_result __d2exp_buffered_n(char* _First, char* const _Last, const double __d,
  uint32_t __precision) {
-  char* const _Original_first = _First;
+    const char* const _Original_first = _First;


Suggested change

const char* const _Original_first = _First;

const char* const _Original_first = _First;

The leading whitespace is unnecessary. Other than that, more const is never a bad thing!

tylerbrawl · 2022-09-07T11:59:56Z

stl/inc/xcharconv_ryu.h

+      uint8_t __lastRemovedDigit = 0;
+      // General case, which happens rarely (~0.7%).


Suggested change

uint8_t __lastRemovedDigit = 0;

// General case, which happens rarely (~0.7%).

// General case, which happens rarely (~0.7%).

uint8_t __lastRemovedDigit = 0;

More bad whitespace detected. Also, I feel like the comment should be above the variable declaration. However, moving __lastRemovedDigit to its most necessary scope is a good change.

tylerbrawl · 2022-09-07T12:04:04Z

stl/inc/xcharconv_ryu.h

@@ -2331,8 +2331,7 @@ _NODISCARD pair<_CharT*, errc> __d2s_buffered_n(_CharT* const _First, _CharT* co
  }

  __floating_decimal_64 __v;
-  const bool __isSmallInt = __d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v);
-  if (__isSmallInt) {
+  if (const bool __isSmallInt = __d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v)) {


Suggested change

if (const bool __isSmallInt = __d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v)) {

if (__d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v)) {

Better yet, just remove __isSmallInt altogether if that is its only purpose. (To be fair, I will do things like const bool isSomethingTrue = [. . .]; in my own code if the defining expression is really long or not very self-explanatory, but the comment beneath the if-statement backs up what is going on here and when this case is hit.)

tylerbrawl · 2022-09-07T12:09:39Z

stl/inc/xlocnum

@@ -1241,7 +1241,7 @@ protected:
        _OutIt _Dest, ios_base& _Iosbase, _Elem _Fill, bool _Val) const { // put formatted bool to _Dest
        if (!(_Iosbase.flags() & ios_base::boolalpha)) {
            return do_put(_Dest, _Iosbase, _Fill, static_cast<long>(_Val));
-        } else { // put "false" or "true"
+        } else { // put "true" or "false"


This is kind of silly, in my opinion. I guess "true or false" rolls off of the tongue better than "false or true," but is something like this really worth the effort?

No changes are suggested here. Keep in mind, however, that reviewers have to look over every change you make. (I still feel bad for the reviewers who had to endure the copious amounts of non-conformance and style differences in my own PR.)

tylerbrawl · 2022-09-07T13:41:48Z

stl/src/xstrxfrm.cpp

-        const int dstlen =
-            __crtLCMapStringA(locale_name, LCMAP_SORTKEY, string2, static_cast<int>(n2), nullptr, 0, codepage, TRUE);
+        const int dstlen = __crtLCMapStringA(
+            locale_name, LCMAP_SORTKEY, string2, static_cast<int>(n2), nullptr, 0, static_cast<int>(codepage), TRUE);


Much like before, the static_cast here is redundant, since the exact same operation is applied with it as without it. It's just a stylistic choice, so I would suggest sticking with what others seem to do in similar scenarios.

A similar situation occurs below.

tylerbrawl · 2022-09-07T13:43:36Z

stl/src/xwcscoll.cpp

-    int ret = 0;
+    int n1 = static_cast<int>(end1 - string1);
+    int n2 = static_cast<int>(end2 - string2);
+    int ret;


This is fine, but perhaps ret could be moved further down to just before its first uses.

No changes are requested here.

tylerbrawl · 2022-09-07T13:47:21Z

stl/src/xwcsxfrm.cpp

@@ -46,7 +46,7 @@ _EXTERN_C_UNLESS_PURE
 //     Non-standard: if OM/API error, return INT_MAX.
 _CRTIMP2_PURE size_t __CLRCALL_PURE_OR_CDECL _Wcsxfrm(_Out_writes_(end1 - string1) _Post_readable_size_(return )
                                                          wchar_t* string1,
-    _In_z_ wchar_t* end1, const wchar_t* string2, const wchar_t* end2, const _Collvec* ploc) {
+    _In_z_ const wchar_t* end1, const wchar_t* string2, const wchar_t* end2, const _Collvec* ploc) {


Suggested change

_In_z_ const wchar_t* end1, const wchar_t* string2, const wchar_t* end2, const _Collvec* ploc) {

_In_z_ wchar_t* end1, const wchar_t* string2, const wchar_t* end2, const _Collvec* ploc) {

Again, the addition of const here, while safer, could break existing code. In an ideal world, we would just be able to add const, and everything would be great. In reality, I can see this causing havoc somewhere, unless you are sure that the function is only called with const wchar_t* values.

Nope, it's not always called with const wchar_t* some devs might need to use it without it being const.

tylerbrawl · 2022-09-07T13:48:40Z

stl/src/xwctomb.cpp

@@ -56,7 +56,7 @@ _CRTIMP2_PURE _Cvtvec __CLRCALL_PURE_OR_CDECL _Getcvt() { // get conversion info
 //     None.

 _CRTIMP2_PURE _Success_(return != -1) int __CLRCALL_PURE_OR_CDECL
-    _Wcrtomb(_Out_ char* s, wchar_t wchar, mbstate_t* pst, const _Cvtvec* ploc) {
+    _Wcrtomb(_Out_ char* s, wchar_t wchar, const mbstate_t* pst, const _Cvtvec* ploc) {


Suggested change

_Wcrtomb(_Out_ char* s, wchar_t wchar, const mbstate_t* pst, const _Cvtvec* ploc) {

_Wcrtomb(_Out_ char* s, wchar_t wchar, mbstate_t* pst, const _Cvtvec* ploc) {

It's just more of the same "this-parameter-really-should-have-const-but-we-can't-just-change-it-now" stuff.

stl/src/xxxprec.hpp

SuperWig · 2022-09-07T15:01:07Z

Please stop squashing and force pushing.

Take a look at this PR #2861 by @StephanTLavavej and take note how he has split the changes into separate commits for ease of review

StephanTLavavej · 2022-09-08T01:34:47Z

Thanks to all of the contributors who have reviewed this PR! I've approved the first batch of fine-grained PRs (#3082, #3083, #3084, #3085, #3086).

I'm going to close this large PR, because further review should happen in smaller PRs.

AZero13 requested a review from a team as a code owner September 5, 2022 18:23

AlexGuteniev suggested changes Sep 5, 2022

View reviewed changes

stl/src/vector_algorithms.cpp Outdated Show resolved Hide resolved

stl/src/stacktrace.cpp Outdated Show resolved Hide resolved

killerswin2 suggested changes Sep 5, 2022

View reviewed changes

stl/src/StlCompareStringA.cpp Outdated Show resolved Hide resolved

stl/src/StlCompareStringA.cpp Outdated Show resolved Hide resolved

AZero13 force-pushed the OK branch 9 times, most recently from fd91cfc to c9d93e6 Compare September 6, 2022 14:22

AraHaan reviewed Sep 6, 2022

View reviewed changes

AZero13 force-pushed the OK branch from e42eb10 to 948d43f Compare September 6, 2022 14:50

AZero13 force-pushed the OK branch 2 times, most recently from 8cf7ff5 to 9c91eee Compare September 6, 2022 16:10

AraHaan mentioned this pull request Sep 6, 2022

Fix C++ warnings microsoft/calculator#1843

Closed

AZero13 force-pushed the OK branch 4 times, most recently from 7ffef35 to f98abc8 Compare September 7, 2022 13:03

AZero13 force-pushed the OK branch 2 times, most recently from f2284bd to dbc16a1 Compare September 7, 2022 13:13

Address C++ Warnings

1534888

Remove unneeded casts, as well as perform simplifications that could improve performance.

AZero13 force-pushed the OK branch from dbc16a1 to 1534888 Compare September 7, 2022 13:18

tylerbrawl reviewed Sep 7, 2022

View reviewed changes

StephanTLavavej mentioned this pull request Sep 8, 2022

Compare the _W_decimal_point of the localeconv, not the decimal_point of said localeconv #3085

Merged

StephanTLavavej closed this Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some C++ warnings #3079

Fix some C++ warnings #3079

AZero13 commented Sep 5, 2022

AlexGuteniev left a comment •

edited

Loading

killerswin2 commented Sep 5, 2022

killerswin2 commented Sep 5, 2022

MattStephanson commented Sep 5, 2022

AraHaan left a comment

sam20908 commented Sep 6, 2022

AraHaan commented Sep 6, 2022 •

edited

Loading

AraHaan commented Sep 6, 2022 •

edited

Loading

StephanTLavavej commented Sep 7, 2022

AZero13 commented Sep 7, 2022

tylerbrawl left a comment

tylerbrawl Sep 7, 2022

tylerbrawl Sep 7, 2022

tylerbrawl Sep 7, 2022

tylerbrawl Sep 7, 2022

tylerbrawl Sep 7, 2022

tylerbrawl Sep 7, 2022

tylerbrawl Sep 7, 2022

AraHaan Sep 7, 2022

tylerbrawl Sep 7, 2022

SuperWig commented Sep 7, 2022

StephanTLavavej commented Sep 8, 2022

	const char* const _Original_first = _First;
	const char* const _Original_first = _First;

		uint8_t __lastRemovedDigit = 0;
		// General case, which happens rarely (~0.7%).

	if (const bool __isSmallInt = __d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v)) {
	if (__d2d_small_int(__ieeeMantissa, __ieeeExponent, &__v)) {

	_In_z_ const wchar_t* end1, const wchar_t* string2, const wchar_t* end2, const _Collvec* ploc) {
	_In_z_ wchar_t* end1, const wchar_t* string2, const wchar_t* end2, const _Collvec* ploc) {

	_Wcrtomb(_Out_ char* s, wchar_t wchar, const mbstate_t* pst, const _Cvtvec* ploc) {
	_Wcrtomb(_Out_ char* s, wchar_t wchar, mbstate_t* pst, const _Cvtvec* ploc) {

Fix some C++ warnings #3079

Fix some C++ warnings #3079

Conversation

AZero13 commented Sep 5, 2022

AlexGuteniev left a comment • edited Loading

Choose a reason for hiding this comment

killerswin2 commented Sep 5, 2022

killerswin2 commented Sep 5, 2022

MattStephanson commented Sep 5, 2022

AraHaan left a comment

Choose a reason for hiding this comment

sam20908 commented Sep 6, 2022

AraHaan commented Sep 6, 2022 • edited Loading

AraHaan commented Sep 6, 2022 • edited Loading

StephanTLavavej commented Sep 7, 2022

AZero13 commented Sep 7, 2022

tylerbrawl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperWig commented Sep 7, 2022

StephanTLavavej commented Sep 8, 2022

AlexGuteniev left a comment •

edited

Loading

AraHaan commented Sep 6, 2022 •

edited

Loading

AraHaan commented Sep 6, 2022 •

edited

Loading