[PATCH] D69479: [Sema] Warn about possible stack exhaution

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

We should resist using `runWithSufficientStackSpace` where possible. Have you tried making this process data-recursive instead? It looks fairly straightforward to form a worklist of expressions to which we want to apply `AnalyseImplicitConversions`, and run through that list until no new entries are added.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3481
+def note_reduce_expression_chaining : Note<
+  "Reduce the number of chained expressions, using helper variables or parens">;
+
----------------
The note should explain what the problem is. If you want to suggest a solution, that could be OK, but should be secondary and non-obvious from the description of the problem. In this case, a note saying the expression is too complex would cover both explaining what the problem is, and implying the solution (make the expression less complex).

Also, Clang diagnostics start with lowercase letters.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:12349
   // This is not the right CC for (e.g.) a variable initialization.
-  AnalyzeImplicitConversions(*this, E, CC);
+  analyzeImplicitConversionsWithSufficientStackSpace(*this, E, CC);
 }
----------------
Mordante wrote:
> xbolva00 wrote:
> > xbolva00 wrote:
> > > Please do not change function names.
> > Ah, you had to change it.
> > 
> > Can this be solved maybe via new bool argument for AnalyzeImplicitConversions(.., arg)? If arg is false, we could just call S.runWithSufficientStackSpace(AnalyzeImplicitConversions(...., true)), otherwise continue in normal flow.
> I like this idea better than my original approach. Thanks for the suggestion.
How about renaming the original function to `AnalyzeImplicitConversionsImpl` and using the original name for the `runWithSufficientStackSpace` version?


================
Comment at: clang/test/Sema/stack-overflow-expr-int.c:2
+// The segmentation fault gives exit code 139
+// RUN: not not %clang_cc1 %s -fsyntax-only 2>&1 | FileCheck %s
+// REQUIRES: thread_support
----------------
You can use `not --crash` if you want to allow crashes. But testing that we do crash seems inappropriate here; if we keep using `runWithSufficientStackSpace`, it would seem preferable to test that we produce the diagnostic, and make sure the test passes regardless of whether we crash.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69479/new/

https://reviews.llvm.org/D69479