On Fri, Oct 25, 2013 at 9:06 AM, Arthur O'Dwyer <[email protected]>wrote:
> IMHO, it would be better to remove the command-line option, and just > set the (hard-coded) limit to 2048 or something else of that general > order of magnitude. I can't think of any valid use for deeply nested > operator->s; it's not like recursive template instantiation, where you > might want to use a higher limit to implement a useful behavior. > The "usual" (albeit very rare) use is to get around the recursive template instantiation depth limit. > Therefore, setting the limit to something in the thousands ought to be > good enough until someone legitimately runs into the limit; and in the > meantime, we're not polluting the command-line docs with obscure and > useless options. > I don't see any problem with setting the default to a higher value, like maybe 2048. > Also FWIW, I do think a DR ought to be opened to make this an explicit > implementation limit in the Standard... but I'm not volunteering. :) > > my $.02, > –Arthur > > > On Fri, Oct 25, 2013 at 7:45 AM, Rahul Jain <[email protected]> > wrote: > > > > Hi, > > > > This is with reference to the below discussion thread: > > > > > http://clang-developers.42468.n3.nabble.com/Endless-operator-gt-chain-causing-infinite-loop-td4035258.html > > > > I have tried to implement a command line argument to handle the issue. > > > > The patch for the same is below: > > > > Index: lib/Driver/Tools.cpp > > =================================================================== > > --- lib/Driver/Tools.cpp (revision 193400) > > +++ lib/Driver/Tools.cpp (working copy) > > @@ -2802,6 +2802,12 @@ > > CmdArgs.push_back(A->getValue()); > > } > > > > + if (Arg *A = Args.getLastArg(options::OPT_foperatorarrow_depth_, > > + options::OPT_foperatorarrow_depth_EQ)) { > > + CmdArgs.push_back("-foperatorarrow-depth"); > Please use foperator_arrow_depth and -foperator-arrow-depth. > > + CmdArgs.push_back(A->getValue()); > > + } > > + > > if (Arg *A = Args.getLastArg(options::OPT_fconstexpr_depth_EQ)) { > > CmdArgs.push_back("-fconstexpr-depth"); > > CmdArgs.push_back(A->getValue()); > > Index: lib/Frontend/CompilerInvocation.cpp > > =================================================================== > > --- lib/Frontend/CompilerInvocation.cpp (revision 193400) > > +++ lib/Frontend/CompilerInvocation.cpp (working copy) > > @@ -1313,6 +1313,8 @@ > > Opts.MathErrno = !Opts.OpenCL && Args.hasArg(OPT_fmath_errno); > > Opts.InstantiationDepth = > > getLastArgIntValue(Args, OPT_ftemplate_depth, 256, Diags); > > + Opts.ArrowDepth = > > + getLastArgIntValue(Args, OPT_foperatorarrow_depth, 256, Diags); > > Opts.ConstexprCallDepth = > > getLastArgIntValue(Args, OPT_fconstexpr_depth, 512, Diags); > > Opts.ConstexprStepLimit = > > Index: lib/Sema/SemaExprCXX.cpp > > =================================================================== > > --- lib/Sema/SemaExprCXX.cpp (revision 193400) > > +++ lib/Sema/SemaExprCXX.cpp (working copy) > > @@ -5188,8 +5188,19 @@ > > llvm::SmallPtrSet<CanQualType,8> CTypes; > > SmallVector<SourceLocation, 8> Locations; > > CTypes.insert(Context.getCanonicalType(BaseType)); > > + > > + static unsigned int arrow_instantiation_count = 0; > This should not be static, and the name should follow the coding convention: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly > + while (BaseType->isRecordType()) { > > + arrow_instantiation_count++; > > + if (arrow_instantiation_count >= getLangOpts().ArrowDepth) { > > + Diag(OpLoc,diag::err_arrow_instantiation_depth_exceeded) > Space after comma. > > + << getLangOpts().ArrowDepth > > + << Base->getSourceRange(); > > + Diag(OpLoc, diag::note_arrow_instantiation_depth) > > + << getLangOpts().ArrowDepth; > > + return ExprError(); > Indentation is strange here. > > + } > > > > - while (BaseType->isRecordType()) { > > Result = BuildOverloadedArrowExpr( > > S, Base, OpLoc, > > // When in a template specialization and on the first loop > > iteration, > > Index: include/clang/Basic/DiagnosticSemaKinds.td > > =================================================================== > > --- include/clang/Basic/DiagnosticSemaKinds.td (revision 193400) > > +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy) > > @@ -2561,6 +2561,11 @@ > > // can handle that case properly. > > def note_ovl_candidate_non_deduced_mismatch_qualified : Note< > > "candidate template ignored: could not match %q0 against %q1">; > > +def err_arrow_instantiation_depth_exceeded : Error< > > + "arrow operator instantiation exceeded maximum depth of %0">, > > + DefaultFatal, NoSFINAE; > We don't need this to be fatal. > > +def note_arrow_instantiation_depth : Note< > > + "use -foperatorarrow-depth=N to increase arrow operator instantiation > > depth">; > > > > // Note that we don't treat templates differently for this diagnostic. > > def note_ovl_candidate_arity : Note<"candidate " > > Index: include/clang/Basic/LangOptions.def > > =================================================================== > > --- include/clang/Basic/LangOptions.def (revision 193400) > > +++ include/clang/Basic/LangOptions.def (working copy) > > @@ -160,6 +160,8 @@ > > ENUM_LANGOPT(SignedOverflowBehavior, SignedOverflowBehaviorTy, 2, > > SOB_Undefined, > > "signed integer overflow handling") > > > > +BENIGN_LANGOPT(ArrowDepth, 32, 256, > > + "maximum arrow instantiation depth") > "instantiation" is not correct here. Maybe "maximum number of operator->s to follow"? > > BENIGN_LANGOPT(InstantiationDepth, 32, 256, > > "maximum template instantiation depth") > > BENIGN_LANGOPT(ConstexprCallDepth, 32, 512, > > Index: include/clang/Driver/CC1Options.td > > =================================================================== > > --- include/clang/Driver/CC1Options.td (revision 193400) > > +++ include/clang/Driver/CC1Options.td (working copy) > > @@ -442,6 +442,8 @@ > > HelpText<"Default type visibility">; > > def ftemplate_depth : Separate<["-"], "ftemplate-depth">, > > HelpText<"Maximum depth of recursive template instantiation">; > > +def foperatorarrow_depth : Separate<["-"], "foperatorarrow-depth">, > > + HelpText<"Maximum depth of arrow operator instantiation">; > > def fconstexpr_depth : Separate<["-"], "fconstexpr-depth">, > > HelpText<"Maximum depth of recursive constexpr function calls">; > > def fconstexpr_steps : Separate<["-"], "fconstexpr-steps">, > > Index: include/clang/Driver/Options.td > > =================================================================== > > --- include/clang/Driver/Options.td (revision 193400) > > +++ include/clang/Driver/Options.td (working copy) > > @@ -775,6 +775,8 @@ > > def ftemplate_depth_ : Joined<["-"], "ftemplate-depth-">, > Group<f_Group>; > > def ftemplate_backtrace_limit_EQ : Joined<["-"], > > "ftemplate-backtrace-limit=">, > > Group<f_Group>; > > +def foperatorarrow_depth_EQ : Joined<["-"], "foperatorarrow-depth=">, > > Group<f_Group>; > > +def foperatorarrow_depth_ : Joined<["-"], "foperatorarrow-depth-">, > > Group<f_Group>; > Please remove this variant. We only support the - form for -ftemplate-depth for compatibility with an old g++ misstep. > > def ftest_coverage : Flag<["-"], "ftest-coverage">, Group<f_Group>; > > def fvectorize : Flag<["-"], "fvectorize">, Group<f_Group>, > > HelpText<"Enable the loop vectorization passes">; > > > > > > > > Please if someone could help me by reviewing it and suggesting > improvements? > It looks basically right, other than the tweaks above. Thanks! Please also add this flag to the documentation.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
