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

[Simon Atanasyan]

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.

--
Simon