clean up list of disabled warnings. by sandreenko · Pull Request #18318 · dotnet/coreclr

sandreenko · 2018-06-05T23:51:41Z

contributes to #18128

For example C4171 was deleted after VS 6.0 (https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-6.0/aa233011(v=vs.60))

because its default value is 4, so this line is useless.

and fix places that trigger it.

sandreenko · 2018-06-06T06:53:00Z

    _ASSERTE(((INT32)(UINT32)n) >= 0);
    _ASSERTE((base > 0) && (base < BITS_PER_SIZE_T));
-    size_t numEncodings = 1 << base;
+    size_t numEncodings = size_t{ 1 } << base;


The previous line checks that base is lower then BITS_PER_SIZE_T, on TARGET_64 it would be 64.
So then if 32 < base < 64 it would produce the wrong result in the old variant.

sandreenko · 2018-06-06T06:55:06Z

                DWORD j;

-                if (IS_ALIGNED(dwCumulativeInstanceFieldPos, 1<<(i+1)))
+                size_t alignment = size_t{ 1 } << i;


There is VS bug that keep showing this warning even when you have an explicit cast to size_t:
size_t alignment = static_cast<size_t>(1 << I); , the issue was reported.

but there is not harm to declare 1 as size_t.

sandreenko · 2018-06-06T06:56:24Z

As usual please review per commit.

sandreenko · 2018-06-06T20:37:38Z

This simple PR affects non only jit, so I am not sure who can review it.
cc @dotnet/jit-contrib .

CarolEidt · 2018-06-07T01:04:01Z

This simple PR affects non only jit, so I am not sure who can review it.

Perhaps @janvorli or @jkotas

jkotas · 2018-06-07T06:31:15Z

What does size_t{ 1 } do? Why is it better than the old fashioned (size_t)1?

janvorli · 2018-06-07T09:59:00Z

                        if (loByte == 0xFF)
                        {
-                            imm8 |= (1 << pos);
+                            imm8 |= (size_t{1} << pos);


The cast here is not necessary, the pos is guaranteed to be less than 8. Doesn't hurt to have it here though. Maybe use ssize_t here to match the target variable type?

Yes, you are right, I have missed the diff between ssize_t and size_t.

janvorli · 2018-06-07T09:59:31Z

                    for (unsigned b = 0; b < 8; b++)
                    {
-                        if (imm & (1 << b))
+                        if (imm & (size_t{1} << b))


The case is not necessary, the loop goes from 0 to 7.

You are right, but because imm has type of ssize_t there is implicit conversion from int to ssize_t here. It generates warning C4334 ('operator' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?), https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334), that was disabled before.

The right thing to do here is to right:
if (imm & static_cast<ssize_t>(1 << b)) , but it also generates warnings, I think it is VC++ compiler issue and I have reported it.

janvorli · 2018-06-07T10:01:18Z

        {
            printf("*");
-            emitDispImm(1 << imm, false);
+            emitDispImm(size_t{1} << imm, false);


The cast is not necessary here, the function asserts at the beginning:
assert((imm >= 0) && (imm <= 4));.
Moreover, the emitDispImm parameter type is int.

coreclr/src/jit/emitarm64.cpp

Line 10284 in a513283

void emitter::emitDispImm(ssize_t imm, bool addComma, bool alwaysHex /* =false */)

janvorli · 2018-06-07T10:05:13Z

                DWORD j;

-                if (IS_ALIGNED(dwCumulativeInstanceFieldPos, 1<<(i+1)))
+                if (IS_ALIGNED(dwCumulativeInstanceFieldPos, size_t{ 1 } << (i + 1)))


The cast is not necessary here, the I goes from 0 to MAX_LOG2_PRIMITIVE_FIELD_SIZE which is 3

janvorli · 2018-06-07T10:05:46Z


                // Place the field
-                dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, 1 << i);
+                dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, size_t{ 1 } << i);


The cast is not necessary here, the I goes from 0 to MAX_LOG2_PRIMITIVE_FIELD_SIZE which is 3

VS team agreed that static_cast<size_t>(1 << I) should not produce the warning, but I think the fix will get some time, so it is better to go with the current solution.

sandreenko · 2018-06-07T20:09:13Z

What does size_t{ 1 } do? Why is it better than the old fashioned (size_t)1?

(size_t)1 is old-style C cast, that is not the best thing to use.
size_t{ 1 } is brace initialization (since C++11), it prevents narrowing conversions (with compile time errors) and is recommended to create constant literals.

sandreenko · 2018-06-14T18:56:41Z

Any additional comments for this PR?

briansull

Looks Good

* delete warnings that do not longer exist For example C4171 was deleted after VS 6.0 (https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-6.0/aa233011(v=vs.60)) * delete C4206 fromm the list because its default value is 4, so this line is useless. * reenable warning as error. * enable warning C4430 and fix places that trigger it. * fix C4334 * format the list * fix ssize_t Commit migrated from dotnet/coreclr@1c8c96e

Sergey Andreenko added 4 commits June 5, 2018 14:46

delete warnings that do not longer exist

bb04c78

For example C4171 was deleted after VS 6.0 (https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-6.0/aa233011(v=vs.60))

delete C4206 fromm the list

9ea2e37

because its default value is 4, so this line is useless.

reenable warning as error.

af50cf9

enable warning C4430

6666f09

and fix places that trigger it.

sandreenko added the 2 - In Progress label Jun 5, 2018

sandreenko force-pushed the cleanWarningsList branch 2 times, most recently from f3cac22 to 85b1478 Compare June 6, 2018 04:39

sandreenko commented Jun 6, 2018

View reviewed changes

Sergey Andreenko added 2 commits June 6, 2018 10:31

fix C4334

ee9a700

format the list

257cc07

sandreenko force-pushed the cleanWarningsList branch from 3f2dcfa to 257cc07 Compare June 6, 2018 18:07

sandreenko changed the title ~~[WIP] clean up list of disabled warnings.~~ clean up list of disabled warnings. Jun 6, 2018

janvorli reviewed Jun 7, 2018

View reviewed changes

fix ssize_t

ce229df

sandreenko mentioned this pull request Jun 14, 2018

fix the desktop build break #18475

Merged

briansull approved these changes Jun 14, 2018

View reviewed changes

sandreenko merged commit 1c8c96e into dotnet:master Jun 15, 2018

sandreenko deleted the cleanWarningsList branch June 15, 2018 01:33

Conversation

sandreenko commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandreenko commented Jun 6, 2018

Uh oh!

sandreenko commented Jun 6, 2018

Uh oh!

CarolEidt commented Jun 7, 2018

Uh oh!

jkotas commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandreenko commented Jun 7, 2018

Uh oh!

sandreenko commented Jun 14, 2018

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sandreenko commented Jun 5, 2018 •

edited

Loading