[cfe-commits] [PATCH] Support MIPS n32 ABI in the Clang driver

[Simon Atanasyan]

On Tue, Oct 16, 2012 at 7:59 AM, Simon Atanasyan <satanasyan at mips.com> wrote:
> On Thu, Oct 11, 2012 at 11:39 PM, Simon Atanasyan <satanasyan at mips.com> wrote:
>> On Thu, Oct 11, 2012 at 7:23 PM, Rafael Espíndola
>> <rafael.espindola at gmail.com> wrote:
>>> On 11 October 2012 10:43, Simon Atanasyan <satanasyan at mips.com> wrote:
>>>> Please review the patch.
>>>
>>> +      StringRef MultiarchSuffix = ::getMultiarchSuffix(TargetArch, Args);
>>>
>>> Why the ::?
>>
>> There is GCCInstallationDetector.getMultiarchSuffix() and I tried to
>> avoid an ambiguity. In the new attached patch I just rename static
>> getMultiarchSuffix to getTargetMultiarchSuffix.
>>
>>> +// FIXME: There is the same routine in the Tools.cpp.
>>> +static bool hasMipsN32ABIArg(const ArgList &Args) {
>>> +  Arg *A = Args.getLastArg(options::OPT_mabi_EQ);
>>> +  return A && (A->getValue(Args) == StringRef("n32"));
>>> +}
>>>
>>> Would making it a method of Generic_GCC fix this?
>>
>> I'm not sure it is better than keeping a tiny code duplication. It
>> definitely removes the code duplication. But it will be the first and
>> the only so specialized function in Generic_GCC. It is not clear why
>> we do not factor out other checking expressions like
>> "Args.hasArg(options::OPT_static)" to the separate functions in the
>> Generic_GCC class.
>>
>>> Why have you moved getMultilibDir? It makes the patch harder to read.
>>
>> I want to group all Mips related helper function in one place. But you
>> are right - that pollutes the patch and should be done in separate
>> commit.
>>
>> Please review new patch.
>
> Ping.
>
> Ping.

--
Simon