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. -- Simon _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
