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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
