[llvm-commits] [PATCH] Add APInt(numBits, ArrayRef<uint64_t> bigVal) constructor (issue4749046)

[Jeffrey Yasskin]

On Mon, Jul 18, 2011 at 11:42 AM, Jeffrey Yasskin <jyasskin at google.com> wrote:
> On Mon, Jul 18, 2011 at 11:33 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> On Mon, Jul 18, 2011 at 9:57 AM, Jeffrey Yasskin <jyasskin at google.com> wrote:
>>> Here's the patch.
>>>
>>> On Sat, Jul 16, 2011 at 10:51 PM,  <jyasskin at gmail.com> wrote:
>>>> Reviewers: eli.friedman_gmail.com,
>>>>
>>>> Message:
>>>> Initial diff at
>>>> http://codereview.appspot.com/download/issue4749046_1.diff
>>>>
>>>> Description:
>>>> Add APInt(numBits, ArrayRef<uint64_t> bigVal) constructor to prevent
>>>> future ambiguity errors like the one corrected by r135261.  Migrate all
>>>> LLVM callers of the old constructor to the new one.
>>>>
>>>>
>>>> Please review this at http://codereview.appspot.com/4749046/
>>>>
>>>> Affected files:
>>>>   M include/llvm/ADT/APInt.h
>>>>   M lib/AsmParser/LLLexer.cpp
>>>>   M lib/Bitcode/Reader/BitcodeReader.cpp
>>>>   M lib/CodeGen/SelectionDAG/FastISel.cpp
>>>>   M lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
>>>>   M lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>>>>   M lib/ExecutionEngine/ExecutionEngine.cpp
>>>>   M lib/Support/APFloat.cpp
>>>>   M lib/Support/APInt.cpp
>>>>   M lib/VMCore/ConstantFold.cpp
>>>>   M lib/VMCore/Core.cpp
>>>>   M unittests/ADT/APIntTest.cpp
>>
>> @@ -342,7 +353,8 @@ public:
>>
>>     if (isSingleWord())
>>       return isUIntN(N, VAL);
>> -    return APInt(N, getNumWords(), pVal).zext(getBitWidth()) == (*this);
>> +    return APInt(N, ArrayRef<uint64_t>(pVal,
>> getNumWords())).zext(getBitWidth())
>> +      == (*this);
>>   }
>>
>>   /// @brief Check if this APInt has an N-bits signed integer value.
>>
>> makeArrayRef just landed; use it. :)  Same in a couple other places.
>
> Silly codebase, changing out from under me. ;-) Will do.
>
>> Otherwise, looks fine.
>>
>> As a followup, could you eliminate both the "APInt(unsigned numBits,
>> unsigned numWords, const uint64_t bigVal[])" and the "APInt(unsigned
>> numBits, uint64_t val, bool isSigned = false)" constructors?  (It
>> should just require ArrayRef-izing and adding an
>> "APInt::getSigned(unsigned numBits, uint64_t val)" helper.)
>
> How do you want to handle dependent codebases? I was thinking to leave
> APInt(...bigVal[]) in for a version to let people switch, but I guess
> LLVM does tend to remove things more aggressively than that. I'm only
> willing to commit to fixing clang myself if you want me to both fix
> things and remove the deprecated constructors.

By which I mean that I'm not volunteering to fix anything besides
clang, but I am happy to fix clang.

Also, do you want APInt::getSigned() to take a uint64_t or an int64_t?
IIRC, taking uint64_t introduces some warnings when we pass literal
negative numbers, and we made ConstantInt::getSigned() take an int64_t
for that reason. Hm, without the bool-taking version of APInt(),
ConstantInt::get() will need an if, or we'll have to change it to not
accept signed numbers either.