Bug 202168 – Address static analysis warnings in B3LowerToAir.cpp: Value stored to '...' during its initialization is never read

• Status: NEW
• Component: JavaScriptCore

[Keith Rollin]

Xcode's static analyzer reports:

    ./b3/B3LowerToAir.cpp:2267:14: warning: Value stored to 'hasFence' during its initialization is never read
            bool hasFence = atomic->hasFence();
                 ^~~~~~~~   ~~~~~~~~~~~~~~~~~~

    ./b3/B3LowerToAir.cpp:2292:13: warning: Value stored to 'expectedValueTmp' during its initialization is never read
            Tmp expectedValueTmp = tmp(atomic->child(0));
                ^~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~

In the first case, "hasFence" is created unconditionally, but only used if "isStrong" is true. Address this by replacing uses of "useFence" with "atomic->hasFence()" and getting rid of "hasFence". "atomic->hasFence()" looks fairly light and so shouldn't be expensive to call. Also, "atomic->hasFence()" is called in other places where it looks like "hasFence" could have been used, so using the former over the latter has precedence.

In the second case, there is an early return in the function between the place "expectedValueTemp" is initialized and where it is used. Address this by moving the initialization down past the early return. This makes the reading of the function a little less elegant, but at least it quiets the warning.

[Radar WebKit Bug Importer]

<rdar://problem/55675530>

[Keith Rollin]

Created attachment 379486 [details]
Patch

[Filip Pizlo]

Comment on attachment 379486 [details]
Patch

These changes make the code style strictly worse.  hasFence exists because we want to have a collection of flags about the current instruction.  expectedValueTmp benefits from being initialized right next to newValueTmp.

Can we just disable these static analysis checks?

[Keith Rollin]

Created attachment 379497 [details]
New patch along the lines that Phil asks.

[Filip Pizlo]

Comment on attachment 379497 [details]
New patch along the lines that Phil asks.

I think this is even uglier than what you had before. :-(

I'd rather just disable the analyzer for this whole file if this is what the fixes look like.

[Yusuke Suzuki]

Comment on attachment 379497 [details]
New patch along the lines that Phil asks.

Yeah, I think this is not good for readability. We should explore the other way, or disable this warning.