[PATCH] D64294: [Driver] Consolidate shouldUseFramePointer() and shouldUseLeafFramePointer()

[Fangrui Song via Phabricator via cfe-commits]

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