MaskRay marked 5 inline comments as done.
MaskRay added inline comments.
================
Comment at: lib/Driver/ToolChains/Clang.cpp:579
-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 bool shouldUseLeafFramePointer(const ArgList &Args,
- const llvm::Triple &Triple) {
- if (Arg *A = Args.getLastArg(options::OPT_mno_omit_leaf_frame_pointer,
- options::OPT_momit_leaf_frame_pointer))
- return A->getOption().matches(options::OPT_mno_omit_leaf_frame_pointer);
-
- if (Args.hasArg(options::OPT_pg))
- return true;
-
- if (Triple.isPS4CPU())
- return false;
-
- return useFramePointerForTargetByDefault(Args, Triple);
+static FramePointerKind getFramePointerKind(const ArgList &Args,
+ const llvm::Triple &Triple) {
----------------
ychen wrote:
> `getFramePointerKind` -> `decideFramePointerKind` /
> `determineFramePointerKind` ?
This is a pure function. I think it is fine to use `get`. This is also
consistent with several other `get*` in this file.
================
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:
> 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;
> }
> ```
>
>
I refactored the code a bit.
I still prefer the current approach to return 3 possible values. Too many
`return` statements just clutter the code I think.
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
