This is an archive of the discontinued LLVM Phabricator instance.

[APInt] Rename getSignBit to getSignMask
ClosedPublic

Authored by craig.topper on Apr 14 2017, 11:35 PM.

Details

Summary

getSignBit is a static function that creates an APInt with only the sign bit set. getSignMask seems like a better name to convey its functionality. In fact several places use it and then store in an APInt named SignMask.

I would like to use the getSignBit name as a method that returns the sign bit of an APInt providing the same information as isNegative(). This would be used in places in ValueTracking that currently use KnownZero.isNegative(). KnownZero and negative in the same statement is somewhat confusing as KnownZero.isNegative() mean that the sign bit is known to be 0 meaning the value being tracked is non-negative.

So rather than having two very different getSignBit methods/functions, I'd like to rename the static one first.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Apr 14 2017, 11:35 PM
spatel accepted this revision.Apr 17 2017, 7:21 AM

The first part of the plan (this patch) makes sense to me, so LGTM. I think I wrote some of the code that already uses a 'SignMask' variable, so it may be worth waiting to hear another opinion. :)

The second part does not make sense to me. If 'getSignBit' is ambiguous/confusing terminology today, then re-introducing the same name with a different meaning will also be confusing going forward. I'd make that 'isSignBitSet/isSignBitClear' to make it less confusing.

This revision is now accepted and ready to land.Apr 17 2017, 7:21 AM
This revision was automatically updated to reflect the committed changes.