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

[Eli Friedman]

On Mon, Jul 18, 2011 at 2:20 PM, Jeffrey Yasskin <jyasskin at google.com> wrote:
> 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.

K; I'll fix llvm-gcc; and it should be easy enough to fix dragonegg.

> 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.

I guess int64_t, since it is expecting a signed value. :)  Making
ConstantInt::get use an if doesn't seem like a big deal.

-Eli