Hi Roman, Looks good. I have two further small comments: (1) There is a stray entry in the options table: + Args.AddLastArg(CmdArgs, options::OPT_fformat_extensions);
(2) Please add some kind of small test to CodeGen for this feature, at the -cc1 level. Otherwise, looks good, please commit! Thanks, - Daniel On Mon, Feb 7, 2011 at 9:35 AM, Roman Divacky <[email protected]> wrote: > hi daniel, > > thnx for review! I fixed all the things you didnt like, updated patches > attached. > > may I commit this? > > I attached two comments below . > >> > + if (!CGM.getCodeGenOpts().GPROFInstrument) >> > + return; >> >> I prefer that tests like this be at the call site, not at the caller. As >> written, this function should be called MaybeEmitMCountInstrumentation(), if >> that makes sense? > > I copied this from EmitFunctionInstrumentation() which does the checking > in the caller - you may want to change that. > >> > - // Explicitly warn that these options are unsupported, even though >> > - // we are allowing compilation to continue. >> > - for (arg_iterator it = Args.filtered_begin(options::OPT_pg), >> > - ie = Args.filtered_end(); it != ie; ++it) { >> > - (*it)->claim(); >> > - D.Diag(clang::diag::warn_drv_clang_unsupported) << >> > (*it)->getAsString(Args); >> > - } >> > + if (Arg *A = Args.getLastArg(options::OPT_pg)) >> > + if (Args.hasArg(options::OPT_fomit_frame_pointer)) >> > + D.Diag(clang::diag::err_drv_argument_not_allowed_with) >> > + << A->getAsString(Args) << "-fomit-frame-pointer"; >> >> I think this error makes more sense if the order is reverse, i.e., >> clang: error: invalid argument '-fomit-frame-pointer' not allowed with >> '-pg' > > agreed. I copied this from darwin toolchain - you may want to change that > there too. > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
