MaskRay marked an inline comment as done. MaskRay added inline comments.
================ Comment at: lib/Driver/ToolChains/Clang.cpp:585 + (A && A->getOption().matches(options::OPT_fno_omit_frame_pointer)) || + (!(A && A->getOption().matches(options::OPT_fomit_frame_pointer)) && + (Args.hasArg(options::OPT_pg) || ---------------- ychen wrote: > It looks better if `frame_pointer` is represented using tri-state. Something > like this? > > It would be great to have comments for conditions that are not obvious such > as the overriding rules. > > ``` > // There are three states for frame_pointer. > enum class FpFlag {true, false, none}; > FpFlag FPF = FpFlag::none; > if (Arg *A = Args.getLastArg(options::OPT_fomit_frame_pointer, > options::OPT_fno_omit_frame_pointer)) > FPF = A->getOption().matches(options::OPT_fno_omit_frame_pointer)) ? > FpFlag::true : FpFlag::false; > > if (!mustUseNonLeaf && FPF == FpFlag::false) > return FramePointerKind::None; > > if (mustUseNonLeaf || FPF == FpFlag::true || Args.hasArg(options::OPT_pg) || > useFramePointerForTargetByDefault(Args, Triple)) { > if (Args.hasFlag(options::OPT_momit_leaf_frame_pointer, > options::OPT_mno_omit_leaf_frame_pointer, > Triple.isPS4CPU())) > return FramePointerKind::NonLeaf; > return FramePointerKind::All; > } > return FramePointerKind::None; > ``` I actually think the current version is clearer.. The local `enum class FpFlag {true, false, none};` doesn't improve readability in my opinion. I can define separate variables for: * A && A->getOption().matches(options::OPT_fno_omit_frame_pointer) * A && A->getOption().matches(options::OPT_fomit_frame_pointer) If reviewers think that makes the code easier to read. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64294/new/ https://reviews.llvm.org/D64294 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits