Bug 74767 – Remove unused variable after r103120 (buildfix)

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Rafael Brandao]

The compiler was complaining about "valueGPR" being set but not used. That patch used that value on its asserts, but right now as it is not used anymore, we're most likely safe to remove it.

[Rafael Brandao]

Created attachment 119697 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119697 [details]
Patch

Clearing flags on attachment: 119697

Committed r103139: <http://trac.webkit.org/changeset/103139>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch massively broke builds :(

[Ryosuke Niwa]

/Volumes/Data/slave/snowleopard-intel-release/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1597:5: error: use of undeclared identifier 'valueGPR' [3]
     ASSERT_UNUSED(valueGPR, valueGPR != property);
     ^
/Volumes/Data/slave/snowleopard-intel-release/build/Source/JavaScriptCore/wtf/Assertions.h:233:51: note: instantiated from:
 #define ASSERT_UNUSED(variable, assertion) ((void)variable)
                                                   ^
/Volumes/Data/slave/snowleopard-intel-release/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1597:19: note: instantiated from:
     ASSERT_UNUSED(valueGPR, valueGPR != property);
                   ^
/Volumes/Data/slave/snowleopard-intel-release/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1745:5: error: use of undeclared identifier 'valueGPR' [3]
     ASSERT_UNUSED(valueGPR, valueGPR != property);
     ^
/Volumes/Data/slave/snowleopard-intel-release/build/Source/JavaScriptCore/wtf/Assertions.h:233:51: note: instantiated from:
 #define ASSERT_UNUSED(variable, assertion) ((void)variable)
                                                   ^
/Volumes/Data/slave/snowleopard-intel-release/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1745:19: note: instantiated from:
     ASSERT_UNUSED(valueGPR, valueGPR != property);
                   ^
2 errors generated.

[Mark Rowe (bdash)]

I rolled it out in r103142 since it’s so obviously incorrect.

[Mark Rowe (bdash)]

It’d be useful to mention which build you’re trying to fix with this change. The build certainly hasn’t been broken since r74747 for any of the major ports we deal with.

[Rafael Brandao]

(In reply to comment #7)
> It’d be useful to mention which build you’re trying to fix with this change. The build certainly hasn’t been broken since r74747 for any of the major ports we deal with.

The patch was fixing the build warning that came with https://bugs.webkit.org/show_bug.cgi?id=74747. It was actually r103120, I've wrongly used the bug id instead in the title. But when it landed, the code that it was fixing was reverted right before, so the fix lost its sense.

[Mark Rowe (bdash)]

Ok. Closing as invalid then since there’s nothing to do here.

[Darin Adler]

The value is used in ASSERT_UNUSED. How did I miss that?

[Csaba Osztrogonác]

(In reply to comment #8)
> (In reply to comment #7)
> > It’d be useful to mention which build you’re trying to fix with this change. The build certainly hasn’t been broken since r74747 for any of the major ports we deal with.
> 
> The patch was fixing the build warning that came with https://bugs.webkit.org/show_bug.cgi?id=74747. It was actually r103120, I've wrongly used the bug id instead in the title. But when it landed, the code that it was fixing was reverted right before, so the fix lost its sense.

Next time it would be better if you add a comment to the original bug and/or add the original bug to the blocks list of the buildfix bug. (sheriffbot does same thing with rollout bug reports)

[Rafael Brandao]

(In reply to comment #11)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > It’d be useful to mention which build you’re trying to fix with this change. The build certainly hasn’t been broken since r74747 for any of the major ports we deal with.
> > 
> > The patch was fixing the build warning that came with https://bugs.webkit.org/show_bug.cgi?id=74747. It was actually r103120, I've wrongly used the bug id instead in the title. But when it landed, the code that it was fixing was reverted right before, so the fix lost its sense.
> 
> Next time it would be better if you add a comment to the original bug and/or add the original bug to the blocks list of the buildfix bug. (sheriffbot does same thing with rollout bug reports)

Ok, I'll do that. And sorry about the trouble with this one.