chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed.
Wow, thanks for the cleanups. This is much easier to read as a consequence. And sorry it took a while to get you another round of review. Comments below. ================ Comment at: include/clang/Driver/CC1Options.td:270 +def mframe_pointer_EQ : Joined<["-"], "mframe-pointer=">, + HelpText<"Specify effect of frame pointer elimination optimization (all,non-leaf,none)">, Values<"all,non-leaf,none">; def mdisable_tail_calls : Flag<["-"], "mdisable-tail-calls">, ---------------- Should say more `Specify which frame pointers to retain (all, non-leaf, none)`. ================ Comment at: lib/Driver/ToolChains/Clang.cpp:574 -static bool shouldUseFramePointer(const ArgList &Args, - const llvm::Triple &Triple) { - if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer, - options::OPT_fomit_frame_pointer)) - return A->getOption().matches(options::OPT_fno_omit_frame_pointer) || - mustUseNonLeafFramePointerForTarget(Triple); - - if (Args.hasArg(options::OPT_pg)) - return true; - - return useFramePointerForTargetByDefault(Args, Triple); -} +static StringRef DetermineFramePointer(const ArgList &Args, + const llvm::Triple &Triple) { ---------------- Keep LLVM coding convention naming pattern please. ================ Comment at: lib/Driver/ToolChains/Clang.cpp:576-579 + Arg *FP = Args.getLastArg(options::OPT_fno_omit_frame_pointer, + options::OPT_fomit_frame_pointer); + Arg *LeafFP = Args.getLastArg(options::OPT_mno_omit_leaf_frame_pointer, + options::OPT_momit_leaf_frame_pointer); ---------------- This still doesn't make sense to me... If the user specifies `-fomit-frame-point` or `-fno-omit-frame-pointer` *after* `-momit-leaf-frame-pointer` or `-mno-omit-leaf-frame-pointer`, then that last flag should win... I think you need to first get the "base" FP state by checking the main two flags. Then you need to get the "last" FP state by checking *all four flags*. When the last flag is a leaf flag, then the state is determined by the base + the last. When the last flag is not one of the leaf flags, then the last flag fully specifies the result. I think you can also process these variables into something simpler to test below, essentially handling all the matching logic in one place. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56353/new/ https://reviews.llvm.org/D56353 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits