Quuxplusone added a comment.
LGTM. I wonder if rsmith is happy with the exact semantics of
"shouldUseUndefinedBehaviorReturnOptimization"... but that seems like a tiny
nit that's fixable post-commit anyway.
================
Comment at: include/clang/Driver/Options.td:1324
+ HelpText<"Use C++ undefined behaviour optimization for control flow paths"
+ "that reach the end of the function without executing a required return">;
+def fno_strict_return : Flag<["-"], "fno-strict-return">, Group<f_Group>,
----------------
Nit: looks like Clang spells it "behavior"?
Nit: I'm still pedantically uncomfortable with describing the strict-return
behavior as an "optimization". I suggest rephrasing this as "-fstrict-return:
Treat control flow paths that fall off the end of a non-void function as
unreachable."
(I notice that neither -fstrict-aliasing nor -fstrict-overflow have any help
text at all.)
================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1173
+ } else if (CGM.getCodeGenOpts().StrictReturn ||
+ shouldUseUndefinedBehaviorReturnOptimization(FD, getContext()))
{
+ if (CGM.getCodeGenOpts().OptimizationLevel == 0)
----------------
arphaman wrote:
> Quuxplusone wrote:
> > I'd expect this to look more like
> >
> > bool ShouldOmitUnreachable = !CGM.getCodeGenOpts().StrictReturn &&
> > FD->getReturnType().isTriviallyCopyableType(Context);
> > // same ifs as the old code
> > if (!ShouldOmitUnreachable) {
> > Builder.CreateUnreachable();
> > Builder.ClearInsertionPoint();
> > }
> >
> > Or in fact, is there a good reason this codepath isn't piggybacking on
> > `FD->hasImplicitReturnZero()`? Why not just give every function
> > `hasImplicitReturnZero` when `-fno-strict-return` is in effect?
> >
> > At which point, `-fno-strict-return` might seem like the wrong name; you
> > could just call it something like `-fimplicit-return-zero`, which would
> > also have the minor benefit of making the `-fno-*` option the default.
> > Or in fact, is there a good reason this codepath isn't piggybacking on
> > FD->hasImplicitReturnZero()? Why not just give every function
> > hasImplicitReturnZero when -fno-strict-return is in effect?
>
> I understand your suggestion, but I'd prefer not to set
> `hasImplicitReturnZero` for all functions, since then Sema won't warn about
> the missing return, which we still want.
>
> > At which point, -fno-strict-return might seem like the wrong name; you
> > could just call it something like -fimplicit-return-zero, which would also
> > have the minor benefit of making the -fno-* option the default.
>
> I don't think a name like "-fimplicit-return-zero" is appropriate, since it
> implies that the compiler guarantees a return of a zero value. This might
> mislead users as they might think that they can use the return value without
> triggering undefined behaviour, which isn't true.
>
> since then Sema won't warn about the missing return
Ah. Yeah, that makes sense.
================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1071
+/// Return true if the given function \p FD should use the undefined behavior
+/// return optimization.
+static bool
----------------
Nit: this code comment is not particularly useful to the reader.
Repository:
rL LLVM
https://reviews.llvm.org/D27163
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
