Page MenuHomePhabricator

Removed unused variable in clang

Authored by nadav on Jul 14 2020, 9:47 AM.



Cleanup the code a bit by removing an unused variable in clang. All of the clang-check tests pass. I discovered this with a Facebook-internal tool.

Diff Detail

Event Timeline

nadav created this revision.Jul 14 2020, 9:47 AM
nadav created this object with visibility "nadav (Nadav Rotem)".
nadav created this object with edit policy "nadav (Nadav Rotem)".
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 9:47 AM
nadav updated this revision to Diff 277883.Jul 14 2020, 9:54 AM
nadav edited the summary of this revision. (Show Details)Jul 14 2020, 10:05 AM
nadav changed the visibility from "nadav (Nadav Rotem)" to "Public (No Login Required)".Jul 14 2020, 10:09 AM
nadav edited reviewers, added: hiraditya; removed: aditya_nandakumar.Jul 14 2020, 11:23 AM

This LGTM! Looks like the last time this variable was touched was in 2010 as part of a mechanical renaming, At that point it was used, as a parameter to the static function MakeObjCStringLiteralFixItHint. That static function was later renamed on Dec 17 2013, to ConversionToObjCStringLiteralCheck, and on the following day Dec 18 2013 in the signature was changed to no longer take a FixitHint parameter. The author must not have removed this variable at that point because technically it is still referenced, but as you point out, it's never mutated in a way that gives the hint a value.

By the way, I tried to accept this diff and leave the following inline comment on SemaExpr.cpp:15799:

I guess this assert was never capable of being hit previously, since the FixItHint::isNull would always return true? I wonder if we'll now see some genuine asserts for ConversionFixItGenerator::isNull thanks to this change.

However when I attempt to do either, Phabricator displays a dialog indicating I don't have permission to "modify" the diff. Not sure what's going on there.

nadav updated this revision to Diff 278031.Jul 14 2020, 5:53 PM
nadav changed the edit policy from "nadav (Nadav Rotem)" to "All Users".Jul 14 2020, 9:19 PM
nadav added a comment.Jul 14 2020, 9:21 PM

@modocache Thank you for the review. I updated the permissions of the diff. Sorry, for the trouble.

modocache accepted this revision.Jul 15 2020, 8:23 AM

No problem, thanks for this!

This revision is now accepted and ready to land.Jul 15 2020, 8:23 AM

Not sure why this didnt get closed after the patch is merged in:

dblaikie closed this revision.Aug 3 2020, 12:02 PM
dblaikie added a subscriber: dblaikie.

Not sure why this didnt get closed after the patch is merged in:

The commit didn't contain the special line " Differential Revision:" - it did link to the review, but without the special prefix, so Phab didn't pick it up and auto-close this review.