Page MenuHomePhabricator

Removed unused variable in clang
ClosedPublic

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

Details

Summary

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.Tue, Jul 14, 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 TranscriptTue, Jul 14, 9:47 AM
nadav updated this revision to Diff 277883.Tue, Jul 14, 9:54 AM
nadav edited the summary of this revision. (Show Details)Tue, Jul 14, 10:05 AM
nadav changed the visibility from "nadav (Nadav Rotem)" to "Public (No Login Required)".Tue, Jul 14, 10:09 AM
nadav edited reviewers, added: hiraditya; removed: aditya_nandakumar.Tue, Jul 14, 11:23 AM

This LGTM! Looks like the last time this variable was touched was in 2010 as part of a mechanical renaming, https://github.com/llvm/llvm-project/commit/a771f46c82d7. At that point it was used, as a parameter to the static function MakeObjCStringLiteralFixItHint. That static function was later renamed on Dec 17 2013 https://github.com/llvm/llvm-project/commit/bd714e9bb12, to ConversionToObjCStringLiteralCheck, and on the following day Dec 18 2013 in https://github.com/llvm/llvm-project/commit/283bf895066 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.Tue, Jul 14, 5:53 PM
nadav changed the edit policy from "nadav (Nadav Rotem)" to "All Users".Tue, Jul 14, 9:19 PM
nadav added a comment.Tue, Jul 14, 9:21 PM

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

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

No problem, thanks for this!

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

Not sure why this didnt get closed after the patch is merged in: https://reviews.llvm.org/rG4fd91b0f946f49370a3ab33c480a807a3f48b247

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

Not sure why this didnt get closed after the patch is merged in: https://reviews.llvm.org/rG4fd91b0f946f49370a3ab33c480a807a3f48b247

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