<div dir="ltr">On Sun, Oct 27, 2013 at 2:10 PM, Rahul Jain <span dir="ltr"><<a href="mailto:1989.rahuljain@gmail.com" target="_blank">1989.rahuljain@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi,<div><br></div><div>Thanks a lot Arthur and Richard for your valuable comments. Feels great when a beginner is supported to such an extent.</div>
<div><br></div><div>As per your comments I have reworked the patch. Here is the revised one. Please take a look for feedback.</div>
<div><br></div><div><div>Index: lib/Frontend/CompilerInvocation.cpp</div><div>===================================================================</div><div>--- lib/Frontend/CompilerInvocation.cpp<span style="white-space:pre-wrap"> </span>(revision 193504)</div>
<div class="im">
<div>+++ lib/Frontend/CompilerInvocation.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -1313,6 +1313,8 @@</div><div> Opts.MathErrno = !Opts.OpenCL && Args.hasArg(OPT_fmath_errno);</div>
<div> Opts.InstantiationDepth =</div><div> getLastArgIntValue(Args, OPT_ftemplate_depth, 256, Diags);</div><div>+ Opts.ArrowDepth =</div></div><div>+ getLastArgIntValue(Args, OPT_foperator_arrow_depth, 2048, Diags);</div>
<div class="im">
<div> Opts.ConstexprCallDepth =</div><div> getLastArgIntValue(Args, OPT_fconstexpr_depth, 512, Diags);</div><div> Opts.ConstexprStepLimit =</div></div><div>Index: lib/Driver/Tools.cpp</div><div>===================================================================</div>
<div>--- lib/Driver/Tools.cpp<span style="white-space:pre-wrap"> </span>(revision 193504)</div><div>+++ lib/Driver/Tools.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -2802,6 +2802,11 @@</div>
<div> CmdArgs.push_back(A->getValue());</div><div> }</div><div> </div><div>+ if (Arg *A = Args.getLastArg(options::OPT_foperator_arrow_depth_EQ)) {</div><div>+ CmdArgs.push_back("-foperator-arrow-depth");</div>
<div class="im">
<div>+ CmdArgs.push_back(A->getValue());</div><div>+ }</div><div>+</div><div> if (Arg *A = Args.getLastArg(options::OPT_fconstexpr_depth_EQ)) {</div><div> CmdArgs.push_back("-fconstexpr-depth");</div>
<div> CmdArgs.push_back(A->getValue());</div></div><div>Index: lib/Sema/SemaExprCXX.cpp</div><div>===================================================================</div><div>--- lib/Sema/SemaExprCXX.cpp<span style="white-space:pre-wrap"> </span>(revision 193504)</div>
<div>+++ lib/Sema/SemaExprCXX.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -5189,7 +5189,17 @@</div><div class="im"><div> SmallVector<SourceLocation, 8> Locations;</div><div> CTypes.insert(Context.getCanonicalType(BaseType));</div>
<div> </div></div><div>+ unsigned int Arrow_instantiation_count = 0;</div></div></div></blockquote><div><br></div><div>This should be CamelCase, without underscores.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div> while (BaseType->isRecordType()) {</div><div>+ Arrow_instantiation_count++;</div><div>+ if (Arrow_instantiation_count >= getLangOpts().ArrowDepth) {</div>
<div>+<span style="white-space:pre-wrap"> </span>Diag(OpLoc, diag::err_arrow_instantiation_depth_exceeded)</div><div>+<span style="white-space:pre-wrap"> </span> << getLangOpts().ArrowDepth << Base->getSourceRange();</div>
<div class="im">
<div>+<span style="white-space:pre-wrap"> </span>Diag(OpLoc, diag::note_arrow_instantiation_depth)</div><div>+<span style="white-space:pre-wrap"> </span> << getLangOpts().ArrowDepth;</div><div>+<span style="white-space:pre-wrap"> </span>return ExprError();</div>
</div></div></div></blockquote><div><br></div><div>The body of this 'if' should be indented by two more spaces.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div class="im">
</div><div class="im"><div>+ }</div><div>+</div><div> Result = BuildOverloadedArrowExpr(</div><div> S, Base, OpLoc,</div><div> // When in a template specialization and on the first loop iteration,</div>
</div><div>Index: include/clang/Basic/LangOptions.def</div>
<div>===================================================================</div><div>--- include/clang/Basic/LangOptions.def<span style="white-space:pre-wrap"> </span>(revision 193504)</div><div class="im"><div>+++ include/clang/Basic/LangOptions.def<span style="white-space:pre-wrap"> </span>(working copy)</div>
<div>@@ -160,6 +160,8 @@</div><div> ENUM_LANGOPT(SignedOverflowBehavior, SignedOverflowBehaviorTy, 2, SOB_Undefined,</div><div> "signed integer overflow handling")</div><div> </div></div><div>+BENIGN_LANGOPT(ArrowDepth, 32, 2048,</div>
<div class="im">
<div>+ "maximum number of operator->s to follow")</div><div> BENIGN_LANGOPT(InstantiationDepth, 32, 256,</div><div> "maximum template instantiation depth")</div><div> BENIGN_LANGOPT(ConstexprCallDepth, 32, 512,</div>
</div><div>Index: include/clang/Basic/DiagnosticSemaKinds.td</div><div>===================================================================</div><div>--- include/clang/Basic/DiagnosticSemaKinds.td<span style="white-space:pre-wrap"> </span>(revision 193504)</div>
<div>+++ include/clang/Basic/DiagnosticSemaKinds.td<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -2569,6 +2569,11 @@</div><div class="im"><div> // can handle that case properly.</div><div> def note_ovl_candidate_non_deduced_mismatch_qualified : Note<</div>
<div> "candidate template ignored: could not match %q0 against %q1">;</div><div>+def err_arrow_instantiation_depth_exceeded : Error<</div></div><div>+ "arrow operator instantiation exceeded maximum depth of %0">;</div>
<div>+def note_arrow_instantiation_depth : Note<</div><div>+ "use -foperator-arrow-depth=N to increase arrow operator instantiation "</div><div>+ "depth">;</div></div></div></blockquote><div>
<br></div><div>Please don't use the word 'instantiation' here.</div><div><br></div><div>Have you considered including notes pointing at the sequence of operator-> functions we called, and the corresponding types? This might not be obvious.</div>
<div><span style="color:rgb(80,0,80)"> </span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div><div class="im"><div> // Note that we don't treat templates differently for this diagnostic.</div>
<div> def note_ovl_candidate_arity : Note<"candidate "</div></div><div>Index: include/clang/Driver/CC1Options.td</div><div>===================================================================</div><div>--- include/clang/Driver/CC1Options.td<span style="white-space:pre-wrap"> </span>(revision 193504)</div>
<div class="im">
<div>+++ include/clang/Driver/CC1Options.td<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -442,6 +442,8 @@</div><div> HelpText<"Default type visibility">;</div><div> def ftemplate_depth : Separate<["-"], "ftemplate-depth">,</div>
<div> HelpText<"Maximum depth of recursive template instantiation">;</div></div><div>+def foperator_arrow_depth : Separate<["-"], "foperator-arrow-depth">,</div><div class="im">
<div>+ HelpText<"Maximum depth of arrow operator instantiation">;</div>
<div> def fconstexpr_depth : Separate<["-"], "fconstexpr-depth">,</div><div> HelpText<"Maximum depth of recursive constexpr function calls">;</div><div> def fconstexpr_steps : Separate<["-"], "fconstexpr-steps">,</div>
<div>Index: include/clang/Driver/Options.td</div><div>===================================================================</div></div><div>--- include/clang/Driver/Options.td<span style="white-space:pre-wrap"> </span>(revision 193504)</div>
<div class="im">
<div>+++ include/clang/Driver/Options.td<span style="white-space:pre-wrap"> </span>(working copy)</div><div>@@ -775,6 +775,8 @@</div><div> def ftemplate_depth_ : Joined<["-"], "ftemplate-depth-">, Group<f_Group>;</div>
<div> def ftemplate_backtrace_limit_EQ : Joined<["-"], "ftemplate-backtrace-limit=">,</div><div> Group<f_Group>;</div></div><div>+def foperator_arrow_depth_EQ : Joined<["-"], "foperator-arrow-depth=">,</div>
<div>+ Group<f_Group>;</div><div class="im"><div> def ftest_coverage : Flag<["-"], "ftest-coverage">, Group<f_Group>;</div><div> def fvectorize : Flag<["-"], "fvectorize">, Group<f_Group>,</div>
<div> HelpText<"Enable the loop vectorization passes">;</div></div></div><div><br></div><div><br></div><div>Attaching the gcc testsuite test case too for reference. </div></div></blockquote><div><br></div>
<div>Please don't mail code from GCC here.</div><div><br></div><div>Instead, you'll need to craft a test case for our test suite. You can take inspiration from</div><div><br></div><div> test/SemaCXX/constexpr-depth.cpp</div>
<div> test/SemaTemplate/instantiation-depth.cpp</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div>How do I add the flag to the documentation?</div></div></blockquote><div><br></div><div>Modify docs/UsersManual.rst</div><div><br></div><div>Thank you for working on this!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div>Thanks,</div><div>Rahul</div></div><div class=""><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Oct 26, 2013 at 12:14 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>On Fri, Oct 25, 2013 at 9:06 AM, Arthur O'Dwyer <span dir="ltr"><<a href="mailto:arthur.j.odwyer@gmail.com" target="_blank">arthur.j.odwyer@gmail.com</a>></span> wrote:<br>
</div><div class="gmail_extra">
<div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">IMHO, it would be better to remove the command-line option, and just<br>
set the (hard-coded) limit to 2048 or something else of that general<br>
order of magnitude. I can't think of any valid use for deeply nested<br>
operator->s; it's not like recursive template instantiation, where you<br>
might want to use a higher limit to implement a useful behavior.<br></blockquote><div><br></div></div><div>The "usual" (albeit very rare) use is to get around the recursive template instantiation depth limit.</div>
<div><div>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Therefore, setting the limit to something in the thousands ought to be<br>
good enough until someone legitimately runs into the limit; and in the<br>
meantime, we're not polluting the command-line docs with obscure and<br>
useless options.<br></blockquote><div><br></div></div><div>I don't see any problem with setting the default to a higher value, like maybe 2048.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Also FWIW, I do think a DR ought to be opened to make this an explicit<br>
implementation limit in the Standard... but I'm not volunteering. :)<br>
<br>
my $.02,<br>
–Arthur<br>
<div><div><br>
<br>
On Fri, Oct 25, 2013 at 7:45 AM, Rahul Jain <<a href="mailto:1989.rahuljain@gmail.com" target="_blank">1989.rahuljain@gmail.com</a>> wrote:<br>
><br>
> Hi,<br>
><br>
> This is with reference to the below discussion thread:<br>
><br>
> <a href="http://clang-developers.42468.n3.nabble.com/Endless-operator-gt-chain-causing-infinite-loop-td4035258.html" target="_blank">http://clang-developers.42468.n3.nabble.com/Endless-operator-gt-chain-causing-infinite-loop-td4035258.html</a><br>
><br>
> I have tried to implement a command line argument to handle the issue.<br>
><br>
> The patch for the same is below:<br>
><br>
> Index: lib/Driver/Tools.cpp<br>
> ===================================================================<br>
> --- lib/Driver/Tools.cpp (revision 193400)<br>
> +++ lib/Driver/Tools.cpp (working copy)<br>
> @@ -2802,6 +2802,12 @@<br>
> CmdArgs.push_back(A->getValue());<br>
> }<br>
><br>
> + if (Arg *A = Args.getLastArg(options::OPT_foperatorarrow_depth_,<br>
> + options::OPT_foperatorarrow_depth_EQ)) {<br>
> + CmdArgs.push_back("-foperatorarrow-depth");<br></div></div></blockquote><div><br></div></div><div>Please use foperator_arrow_depth and -foperator-arrow-depth.</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>
> + CmdArgs.push_back(A->getValue());<br>
> + }<br>
> +<br>
> if (Arg *A = Args.getLastArg(options::OPT_fconstexpr_depth_EQ)) {<br>
> CmdArgs.push_back("-fconstexpr-depth");<br>
> CmdArgs.push_back(A->getValue());<br>
> Index: lib/Frontend/CompilerInvocation.cpp<br>
> ===================================================================<br>
> --- lib/Frontend/CompilerInvocation.cpp (revision 193400)<br>
> +++ lib/Frontend/CompilerInvocation.cpp (working copy)<br>
> @@ -1313,6 +1313,8 @@<br>
> Opts.MathErrno = !Opts.OpenCL && Args.hasArg(OPT_fmath_errno);<br>
> Opts.InstantiationDepth =<br>
> getLastArgIntValue(Args, OPT_ftemplate_depth, 256, Diags);<br>
> + Opts.ArrowDepth =<br>
> + getLastArgIntValue(Args, OPT_foperatorarrow_depth, 256, Diags);<br>
> Opts.ConstexprCallDepth =<br>
> getLastArgIntValue(Args, OPT_fconstexpr_depth, 512, Diags);<br>
> Opts.ConstexprStepLimit =<br>
> Index: lib/Sema/SemaExprCXX.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaExprCXX.cpp (revision 193400)<br>
> +++ lib/Sema/SemaExprCXX.cpp (working copy)<br>
> @@ -5188,8 +5188,19 @@<br>
> llvm::SmallPtrSet<CanQualType,8> CTypes;<br>
> SmallVector<SourceLocation, 8> Locations;<br>
> CTypes.insert(Context.getCanonicalType(BaseType));<br>
> +<br>
> + static unsigned int arrow_instantiation_count = 0;<br></div></div></blockquote><div><br></div></div></div><div>This should not be static, and the name should follow the coding convention:</div><div><br></div><div>
<a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly" target="_blank">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a></div>
<div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>
> + while (BaseType->isRecordType()) {<br>
> + arrow_instantiation_count++;<br>
> + if (arrow_instantiation_count >= getLangOpts().ArrowDepth) {<br>
> + Diag(OpLoc,diag::err_arrow_instantiation_depth_exceeded)<br></div></div></blockquote><div><br></div></div><div>Space after comma.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>
> + << getLangOpts().ArrowDepth<br>
> + << Base->getSourceRange();<br>
> + Diag(OpLoc, diag::note_arrow_instantiation_depth)<br>
> + << getLangOpts().ArrowDepth;<br>
> + return ExprError();<br></div></div></blockquote><div><br></div></div><div>Indentation is strange here.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>
> + }<br>
><br>
> - while (BaseType->isRecordType()) {<br>
> Result = BuildOverloadedArrowExpr(<br>
> S, Base, OpLoc,<br>
> // When in a template specialization and on the first loop<br>
> iteration,<br>
> Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
> ===================================================================<br>
> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 193400)<br>
> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)<br>
> @@ -2561,6 +2561,11 @@<br>
> // can handle that case properly.<br>
> def note_ovl_candidate_non_deduced_mismatch_qualified : Note<<br>
> "candidate template ignored: could not match %q0 against %q1">;<br>
> +def err_arrow_instantiation_depth_exceeded : Error<<br>
> + "arrow operator instantiation exceeded maximum depth of %0">,<br>
> + DefaultFatal, NoSFINAE;<br></div></div></blockquote><div><br></div></div><div>We don't need this to be fatal.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>
> +def note_arrow_instantiation_depth : Note<<br>
> + "use -foperatorarrow-depth=N to increase arrow operator instantiation<br>
> depth">;<br>
><br>
> // Note that we don't treat templates differently for this diagnostic.<br>
> def note_ovl_candidate_arity : Note<"candidate "<br>
> Index: include/clang/Basic/LangOptions.def<br>
> ===================================================================<br>
> --- include/clang/Basic/LangOptions.def (revision 193400)<br>
> +++ include/clang/Basic/LangOptions.def (working copy)<br>
> @@ -160,6 +160,8 @@<br>
> ENUM_LANGOPT(SignedOverflowBehavior, SignedOverflowBehaviorTy, 2,<br>
> SOB_Undefined,<br>
> "signed integer overflow handling")<br>
><br>
> +BENIGN_LANGOPT(ArrowDepth, 32, 256,<br>
> + "maximum arrow instantiation depth")<br></div></div></blockquote><div><br></div></div><div>"instantiation" is not correct here. Maybe "maximum number of operator->s to follow"?</div>
<div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>
> BENIGN_LANGOPT(InstantiationDepth, 32, 256,<br>
> "maximum template instantiation depth")<br>
> BENIGN_LANGOPT(ConstexprCallDepth, 32, 512,<br>
> Index: include/clang/Driver/CC1Options.td<br>
> ===================================================================<br>
> --- include/clang/Driver/CC1Options.td (revision 193400)<br>
> +++ include/clang/Driver/CC1Options.td (working copy)<br>
> @@ -442,6 +442,8 @@<br>
> HelpText<"Default type visibility">;<br>
> def ftemplate_depth : Separate<["-"], "ftemplate-depth">,<br>
> HelpText<"Maximum depth of recursive template instantiation">;<br>
> +def foperatorarrow_depth : Separate<["-"], "foperatorarrow-depth">,<br>
> + HelpText<"Maximum depth of arrow operator instantiation">;<br>
> def fconstexpr_depth : Separate<["-"], "fconstexpr-depth">,<br>
> HelpText<"Maximum depth of recursive constexpr function calls">;<br>
> def fconstexpr_steps : Separate<["-"], "fconstexpr-steps">,<br>
> Index: include/clang/Driver/Options.td<br>
> ===================================================================<br>
> --- include/clang/Driver/Options.td (revision 193400)<br>
> +++ include/clang/Driver/Options.td (working copy)<br>
> @@ -775,6 +775,8 @@<br>
> def ftemplate_depth_ : Joined<["-"], "ftemplate-depth-">, Group<f_Group>;<br>
> def ftemplate_backtrace_limit_EQ : Joined<["-"],<br>
> "ftemplate-backtrace-limit=">,<br>
> Group<f_Group>;<br>
> +def foperatorarrow_depth_EQ : Joined<["-"], "foperatorarrow-depth=">,<br>
> Group<f_Group>;<br>
> +def foperatorarrow_depth_ : Joined<["-"], "foperatorarrow-depth-">,<br>
> Group<f_Group>;<br></div></div></blockquote><div><br></div></div></div><div>Please remove this variant. We only support the - form for -ftemplate-depth for compatibility with an old g++ misstep.</div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>
> def ftest_coverage : Flag<["-"], "ftest-coverage">, Group<f_Group>;<br>
> def fvectorize : Flag<["-"], "fvectorize">, Group<f_Group>,<br>
> HelpText<"Enable the loop vectorization passes">;<br>
><br>
><br>
><br>
> Please if someone could help me by reviewing it and suggesting improvements?</div></div></blockquote><div><br></div></div><div>It looks basically right, other than the tweaks above. Thanks!</div><div><br></div><div>
Please also add this flag to the documentation. </div>
</div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>