On Tue, Oct 16, 2012 at 7:59 AM, Simon Atanasyan <[email protected]> wrote: > On Thu, Oct 11, 2012 at 11:39 PM, Simon Atanasyan <[email protected]> wrote: >> On Thu, Oct 11, 2012 at 7:23 PM, Rafael Espíndola >> <[email protected]> wrote: >>> On 11 October 2012 10:43, Simon Atanasyan <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
