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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen 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) ||
----------------
MaskRay wrote:
> 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.
I think local enum may be optional.

Say 
  - `Fp      = A && A->getOption().matches(options::OPT_fno_omit_frame_pointer)`
  - `NoFp = A && A->getOption().matches(options::OPT_fomit_frame_pointer)`

The `!(A && A->getOption().matches(options::OPT_fomit_frame_pointer))` in the current revision could be `!A`. The implicit logic is `NoFp`  could only be overriden by `mustUseNonLeaf`.

This block helps to make the implicit logic explicit and simplify the rest of the code.

```
if (!mustUseNonLeaf && NoFp)
  return FramePointerKind::None;
}
```




Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64294/new/

https://reviews.llvm.org/D64294