[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1da13237a41a: [Diagnostics] Try to improve warning message 
for -Wreturn-type (authored by xbolva00).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/test/Analysis/const-method-call.cpp
  clang/test/Analysis/structured_bindings.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p7.cpp
  clang/test/Driver/cc-log-diagnostics.c
  clang/test/Frontend/absolute-paths.c
  clang/test/Index/warning-flags.c
  clang/test/Misc/serialized-diags-stable.c
  clang/test/Modules/redecl-merge.m
  clang/test/PCH/late-parsed-instantiations.cpp
  clang/test/Sema/block-return-1.c
  clang/test/Sema/block-return-3.c
  clang/test/Sema/freemain.c
  clang/test/Sema/return.c
  clang/test/SemaCXX/attr-noreturn.cpp
  clang/test/SemaCXX/coreturn.cpp
  clang/test/SemaCXX/return-noreturn.cpp
  clang/test/SemaCXX/warn-missing-noreturn.cpp
  clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp

Index: clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp
===
--- clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp
+++ clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp
@@ -15,8 +15,8 @@
   char data() {
 visit([](auto buffer) -> char { // expected-note {{in instantiation}}
   buffer->data();
-}); // expected-warning {{control reaches end of non-void lambda}}
-  } // expected-warning {{control reaches end of non-void function}}
+}); // expected-warning {{non-void lambda does not return a value}}
+  } // expected-warning {{non-void function does not return a value}}
 };
 
 // pr34185
Index: clang/test/SemaCXX/warn-missing-noreturn.cpp
===
--- clang/test/SemaCXX/warn-missing-noreturn.cpp
+++ clang/test/SemaCXX/warn-missing-noreturn.cpp
@@ -91,7 +91,7 @@
 
 int rdar8875247_test() {
   rdar8875247 f;
-} // expected-warning{{control reaches end of non-void function}}
+} // expected-warning{{non-void function does not return a value}}
 
 struct rdar8875247_B {
   rdar8875247_B();
Index: clang/test/SemaCXX/return-noreturn.cpp
===
--- clang/test/SemaCXX/return-noreturn.cpp
+++ clang/test/SemaCXX/return-noreturn.cpp
@@ -26,7 +26,7 @@
   }
   int f2_positive(int x) {
 switch (x) { default: ; }
-  } // expected-warning {{control reaches end of non-void function}}
+  } // expected-warning {{non-void function does not return a value}}
   int f3(int x) {
 switch (x) { default: { pr6884_abort_struct(); } }
   }
@@ -46,7 +46,7 @@
   pr6884_abort_struct *p = new pr6884_abort_struct();
   delete p;
 }
-  } // expected-warning {{control reaches end of non-void function}}
+  } // expected-warning {{non-void function does not return a value}}
 
   // Test that these constructs work even when extraneous blocks are created
   // before and after the switch due to implicit destructors.
@@ -61,7 +61,7 @@
   int g2_positive(int x) {
 other o;
 switch (x) { default: ; }
-  } // expected-warning {{control reaches end of non-void function}}
+  } // expected-warning {{non-void function does not return a value}}
   int g3(int x) {
 other o;
 switch (x) { default: { pr6884_abort_struct(); } }
@@ -140,7 +140,7 @@
 default:
 break;
   }
-} // expected-warning {{control reaches end of non-void function}}
+} // expected-warning {{non-void function does not return a value}}
 
 void PR9412_f() {
 PR9412_t(); // expected-note {{in instantiation of function template specialization 'PR9412_t' requested here}}
@@ -165,7 +165,7 @@
 
 int testTernaryStaticallyConditionalRetrunOnTrue() {
   true ? Return() : NoReturn();
-} // expected-warning {{control reaches end of non-void function}}
+} // expected-warning {{non-void function does not return a value}}
 
 int testTernaryStaticallyConditionalNoretrunOnFalse() {
   false ? Return() : NoReturn();
@@ -173,23 +173,23 @@
 
 int testTernaryStaticallyConditionalRetrunOnFalse() {
   false ? NoReturn() : Return();
-} // expected-warning {{control reaches end of non-void function}}
+} // expected-warning {{non-void function does not return a value}}
 
 int testTernaryConditionalNoreturnTrueBranch(bool value) {
   value ? (NoReturn() || NoReturn()) : Return();
-} // expected-warning {{control may reach end of non-void function}}
+} // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testTernaryConditionalNoreturnFalseBranch(bool value) {
   value ? Return() : (NoReturn() || NoReturn());
-} // expected-warning {{control may reach end of non-void function}}
+} // expected-warning {{non-void function does not return a v

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 228578.
xbolva00 added a comment.
Herald added a subscriber: arphaman.

Added FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/test/Analysis/const-method-call.cpp
  clang/test/Analysis/structured_bindings.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p7.cpp
  clang/test/Driver/cc-log-diagnostics.c
  clang/test/Frontend/absolute-paths.c
  clang/test/Index/warning-flags.c
  clang/test/Misc/serialized-diags-stable.c
  clang/test/Modules/redecl-merge.m
  clang/test/PCH/late-parsed-instantiations.cpp
  clang/test/Sema/block-return-1.c
  clang/test/Sema/block-return-3.c
  clang/test/Sema/freemain.c
  clang/test/Sema/return.c
  clang/test/SemaCXX/attr-noreturn.cpp
  clang/test/SemaCXX/coreturn.cpp
  clang/test/SemaCXX/return-noreturn.cpp
  clang/test/SemaCXX/warn-missing-noreturn.cpp
  clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp

Index: clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp
===
--- clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp
+++ clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp
@@ -15,8 +15,8 @@
   char data() {
 visit([](auto buffer) -> char { // expected-note {{in instantiation}}
   buffer->data();
-}); // expected-warning {{control reaches end of non-void lambda}}
-  } // expected-warning {{control reaches end of non-void function}}
+}); // expected-warning {{non-void lambda does not return a value}}
+  } // expected-warning {{non-void function does not return a value}}
 };
 
 // pr34185
Index: clang/test/SemaCXX/warn-missing-noreturn.cpp
===
--- clang/test/SemaCXX/warn-missing-noreturn.cpp
+++ clang/test/SemaCXX/warn-missing-noreturn.cpp
@@ -91,7 +91,7 @@
 
 int rdar8875247_test() {
   rdar8875247 f;
-} // expected-warning{{control reaches end of non-void function}}
+} // expected-warning{{non-void function does not return a value}}
 
 struct rdar8875247_B {
   rdar8875247_B();
Index: clang/test/SemaCXX/return-noreturn.cpp
===
--- clang/test/SemaCXX/return-noreturn.cpp
+++ clang/test/SemaCXX/return-noreturn.cpp
@@ -26,7 +26,7 @@
   }
   int f2_positive(int x) {
 switch (x) { default: ; }
-  } // expected-warning {{control reaches end of non-void function}}
+  } // expected-warning {{non-void function does not return a value}}
   int f3(int x) {
 switch (x) { default: { pr6884_abort_struct(); } }
   }
@@ -46,7 +46,7 @@
   pr6884_abort_struct *p = new pr6884_abort_struct();
   delete p;
 }
-  } // expected-warning {{control reaches end of non-void function}}
+  } // expected-warning {{non-void function does not return a value}}
 
   // Test that these constructs work even when extraneous blocks are created
   // before and after the switch due to implicit destructors.
@@ -61,7 +61,7 @@
   int g2_positive(int x) {
 other o;
 switch (x) { default: ; }
-  } // expected-warning {{control reaches end of non-void function}}
+  } // expected-warning {{non-void function does not return a value}}
   int g3(int x) {
 other o;
 switch (x) { default: { pr6884_abort_struct(); } }
@@ -140,7 +140,7 @@
 default:
 break;
   }
-} // expected-warning {{control reaches end of non-void function}}
+} // expected-warning {{non-void function does not return a value}}
 
 void PR9412_f() {
 PR9412_t(); // expected-note {{in instantiation of function template specialization 'PR9412_t' requested here}}
@@ -165,7 +165,7 @@
 
 int testTernaryStaticallyConditionalRetrunOnTrue() {
   true ? Return() : NoReturn();
-} // expected-warning {{control reaches end of non-void function}}
+} // expected-warning {{non-void function does not return a value}}
 
 int testTernaryStaticallyConditionalNoretrunOnFalse() {
   false ? Return() : NoReturn();
@@ -173,23 +173,23 @@
 
 int testTernaryStaticallyConditionalRetrunOnFalse() {
   false ? NoReturn() : Return();
-} // expected-warning {{control reaches end of non-void function}}
+} // expected-warning {{non-void function does not return a value}}
 
 int testTernaryConditionalNoreturnTrueBranch(bool value) {
   value ? (NoReturn() || NoReturn()) : Return();
-} // expected-warning {{control may reach end of non-void function}}
+} // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testTernaryConditionalNoreturnFalseBranch(bool value) {
   value ? Return() : (NoReturn() || NoReturn());
-} // expected-warning {{control may reach end of non-void function}}
+} // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testConditionallyExecutedComplexTer

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6860
 "lambda declared 'noreturn' should not return">;
   def warn_maybe_falloff_nonvoid_lambda : Warning<
+"non-void lambda does not return a value in all control paths">,

xbolva00 wrote:
> aaron.ballman wrote:
> > And maybe move these two up closer to the other duplicate-ish diagnostics.
> This is in
> "let CategoryName = "Lambda Issue" in {"
> 
> so I am not sure if should really move it.
I hadn't noticed that, let's leave it here. Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6860
 "lambda declared 'noreturn' should not return">;
   def warn_maybe_falloff_nonvoid_lambda : Warning<
+"non-void lambda does not return a value in all control paths">,

aaron.ballman wrote:
> And maybe move these two up closer to the other duplicate-ish diagnostics.
This is in
"let CategoryName = "Lambda Issue" in {"

so I am not sure if should really move it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D69762#1739022 , @xbolva00 wrote:

> We cannot use it for block; it is a hard error. Corountine and function can 
> be merged as suggested.


We can use `.Text` to share diagnostic text between warnings and errors (we do 
this elsewhere), but I don't insist for this patch.

LGTM aside from a commenting request.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:578
 
 def warn_maybe_falloff_nonvoid_function : Warning<
+  "non-void function does not return a value in all control paths">,

A FIXME comment that we'd like to combine these diagnostics eventually would be 
appreciated.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6860
 "lambda declared 'noreturn' should not return">;
   def warn_maybe_falloff_nonvoid_lambda : Warning<
+"non-void lambda does not return a value in all control paths">,

And maybe move these two up closer to the other duplicate-ish diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 228514.
xbolva00 added a comment.

Fixed review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/test/Sema/block-return-1.c
  clang/test/Sema/block-return-3.c
  clang/test/Sema/freemain.c
  clang/test/Sema/return.c
  clang/test/SemaCXX/attr-noreturn.cpp
  clang/test/SemaCXX/coreturn.cpp
  clang/test/SemaCXX/return-noreturn.cpp
  clang/test/SemaCXX/warn-missing-noreturn.cpp
  clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp

Index: clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp
===
--- clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp
+++ clang/test/SemaTemplate/late-parsing-eager-instantiation.cpp
@@ -15,8 +15,8 @@
   char data() {
 visit([](auto buffer) -> char { // expected-note {{in instantiation}}
   buffer->data();
-}); // expected-warning {{control reaches end of non-void lambda}}
-  } // expected-warning {{control reaches end of non-void function}}
+}); // expected-warning {{non-void lambda does not return a value}}
+  } // expected-warning {{non-void function does not return a value}}
 };
 
 // pr34185
Index: clang/test/SemaCXX/warn-missing-noreturn.cpp
===
--- clang/test/SemaCXX/warn-missing-noreturn.cpp
+++ clang/test/SemaCXX/warn-missing-noreturn.cpp
@@ -91,7 +91,7 @@
 
 int rdar8875247_test() {
   rdar8875247 f;
-} // expected-warning{{control reaches end of non-void function}}
+} // expected-warning{{non-void function does not return a value}}
 
 struct rdar8875247_B {
   rdar8875247_B();
Index: clang/test/SemaCXX/return-noreturn.cpp
===
--- clang/test/SemaCXX/return-noreturn.cpp
+++ clang/test/SemaCXX/return-noreturn.cpp
@@ -26,7 +26,7 @@
   }
   int f2_positive(int x) {
 switch (x) { default: ; }
-  } // expected-warning {{control reaches end of non-void function}}
+  } // expected-warning {{non-void function does not return a value}}
   int f3(int x) {
 switch (x) { default: { pr6884_abort_struct(); } }
   }
@@ -46,7 +46,7 @@
   pr6884_abort_struct *p = new pr6884_abort_struct();
   delete p;
 }
-  } // expected-warning {{control reaches end of non-void function}}
+  } // expected-warning {{non-void function does not return a value}}
 
   // Test that these constructs work even when extraneous blocks are created
   // before and after the switch due to implicit destructors.
@@ -61,7 +61,7 @@
   int g2_positive(int x) {
 other o;
 switch (x) { default: ; }
-  } // expected-warning {{control reaches end of non-void function}}
+  } // expected-warning {{non-void function does not return a value}}
   int g3(int x) {
 other o;
 switch (x) { default: { pr6884_abort_struct(); } }
@@ -140,7 +140,7 @@
 default:
 break;
   }
-} // expected-warning {{control reaches end of non-void function}}
+} // expected-warning {{non-void function does not return a value}}
 
 void PR9412_f() {
 PR9412_t(); // expected-note {{in instantiation of function template specialization 'PR9412_t' requested here}}
@@ -165,7 +165,7 @@
 
 int testTernaryStaticallyConditionalRetrunOnTrue() {
   true ? Return() : NoReturn();
-} // expected-warning {{control reaches end of non-void function}}
+} // expected-warning {{non-void function does not return a value}}
 
 int testTernaryStaticallyConditionalNoretrunOnFalse() {
   false ? Return() : NoReturn();
@@ -173,23 +173,23 @@
 
 int testTernaryStaticallyConditionalRetrunOnFalse() {
   false ? NoReturn() : Return();
-} // expected-warning {{control reaches end of non-void function}}
+} // expected-warning {{non-void function does not return a value}}
 
 int testTernaryConditionalNoreturnTrueBranch(bool value) {
   value ? (NoReturn() || NoReturn()) : Return();
-} // expected-warning {{control may reach end of non-void function}}
+} // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testTernaryConditionalNoreturnFalseBranch(bool value) {
   value ? Return() : (NoReturn() || NoReturn());
-} // expected-warning {{control may reach end of non-void function}}
+} // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testConditionallyExecutedComplexTernaryTrueBranch(bool value) {
   value || (true ? NoReturn() : true);
-} // expected-warning {{control may reach end of non-void function}}
+} // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testConditionallyExecutedComplexTernaryFalseBranch(bool value) {
   value || (false ? true : NoReturn());
-} // expected-warning {{control may reach end of non-void function}}
+} // expected-warning {{non-void function does not return a

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Looking at Sema/AnalysisBasedWarnings.cpp, reworking it to "select" would take 
some time so I will prepare just simple patch without "select".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

We cannot use it for block; it is a hard error. Corountine and function can be 
merged as suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'd probably go with `"non-void %select{function|block|coroutine}0 does not 
return a value %select{|in all control paths}1"`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

I am not sure if "may" is ideal, it could suggest that compiler is not very 
sure and emits just some conservative message.

"not all paths in this non-void {function,block} return a value" gives some 
extra confidence that compiler knows there is a buggy control path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In D69762#1737612 , @easyaspi314 wrote:

> How about "this non-void {function|block} {may|does} not return a value"


FWIW, I am happy with this clear and concise suggestion. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[easyaspi314 (Devin) via Phabricator via cfe-commits]

easyaspi314 added a comment.

How about "this non-void {function|block} {may|does} not return a value"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:579
 def warn_maybe_falloff_nonvoid_function : Warning<
-  "control may reach end of non-void function">,
+  "not all control paths in this function return a value; non-void function 
must return a value">,
   InGroup;

Quuxplusone wrote:
> As long as we're messing with this wording: Does it actually help any human 
> reader to distinguish "control paths" versus simply "paths"? And could we DRY 
> it up by saying
> 
> > not all paths in this non-void {function,block} return a value
> 
> > this non-void {function,block} does not return a value
> 
> > not all paths in this coroutine return a value, and the promise type %0 
> > does not declare 'return_void()'
> 
> > this coroutine does not return a value, and the promise type %0 does not 
> > declare 'return_void()'
> 
> I don't think the Coroutines warning needs to specifically call out 
> "undefined behavior," unless it is trying to say that the code is IFNDR. //Of 
> course// falling off the end of a function is UB if it ever actually happens 
> at runtime; that's no different whether it's a coroutine or a regular 
> function/block. The only reason for a wording difference in the Coroutines 
> case is that the colloquial notion of a "(non-)void coroutine" (whose return 
> type would be something like `task`) is slightly less familiar than the 
> colloquial notion of a "(non-)void function" (whose return type is literally 
> `void`).
I am also wondering if we could use some better term than "control paths", but 
I haven't found any :)  Paths sound to general for me but I am not strongly 
opposed.

if @aaron.ballman  is happy with your wording, so do I.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:579
 def warn_maybe_falloff_nonvoid_function : Warning<
-  "control may reach end of non-void function">,
+  "not all control paths in this function return a value; non-void function 
must return a value">,
   InGroup;

As long as we're messing with this wording: Does it actually help any human 
reader to distinguish "control paths" versus simply "paths"? And could we DRY 
it up by saying

> not all paths in this non-void {function,block} return a value

> this non-void {function,block} does not return a value

> not all paths in this coroutine return a value, and the promise type %0 does 
> not declare 'return_void()'

> this coroutine does not return a value, and the promise type %0 does not 
> declare 'return_void()'

I don't think the Coroutines warning needs to specifically call out "undefined 
behavior," unless it is trying to say that the code is IFNDR. //Of course// 
falling off the end of a function is UB if it ever actually happens at runtime; 
that's no different whether it's a coroutine or a regular function/block. The 
only reason for a wording difference in the Coroutines case is that the 
colloquial notion of a "(non-)void coroutine" (whose return type would be 
something like `task`) is slightly less familiar than the colloquial 
notion of a "(non-)void function" (whose return type is literally `void`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:584-587
 def err_maybe_falloff_nonvoid_block : Error<
   "control may reach end of non-void block">;
 def err_falloff_nonvoid_block : Error<
   "control reaches end of non-void block">;

xbolva00 wrote:
> aaron.ballman wrote:
> > Should we change this wording as well? Along with any other instances of 
> > similar wording (I don't recall if we have something similar for lambdas or 
> > if we defer to the function diagnostics in that case)?
> +1, I agree.
> 
> It would be better if somebody more familiar with  Obj-C / coroutines and 
> related terminology would improve these text.
I think you've already got the gist of it. `block does not return a value; 
non-void blocks must return a value`, and similar for coroutines.

In fact, I wonder why we don't just combine these diagnostics for all three 
situations and use a `%select` for them? That seems like a reasonable thing to 
do (having not put a ton of thought into it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:584-587
 def err_maybe_falloff_nonvoid_block : Error<
   "control may reach end of non-void block">;
 def err_falloff_nonvoid_block : Error<
   "control reaches end of non-void block">;

aaron.ballman wrote:
> Should we change this wording as well? Along with any other instances of 
> similar wording (I don't recall if we have something similar for lambdas or 
> if we defer to the function diagnostics in that case)?
+1, I agree.

It would be better if somebody more familiar with  Obj-C / coroutines and 
related terminology would improve these text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think the new text is more clear than the old text, so I'm fine with the 
current direction you're going.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:584-587
 def err_maybe_falloff_nonvoid_block : Error<
   "control may reach end of non-void block">;
 def err_falloff_nonvoid_block : Error<
   "control reaches end of non-void block">;

Should we change this wording as well? Along with any other instances of 
similar wording (I don't recall if we have something similar for lambdas or if 
we defer to the function diagnostics in that case)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 227611.
xbolva00 added a comment.

Up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td


Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -576,10 +576,10 @@
   "thread-local storage is not supported for the current target">;
 
 def warn_maybe_falloff_nonvoid_function : Warning<
-  "control may reach end of non-void function">,
+  "not all control paths in this function return a value; non-void function 
must return a value">,
   InGroup;
 def warn_falloff_nonvoid_function : Warning<
-  "control reaches end of non-void function">,
+  "function does not return a value; non-void function must return a value">,
   InGroup;
 def err_maybe_falloff_nonvoid_block : Error<
   "control may reach end of non-void block">;


Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -576,10 +576,10 @@
   "thread-local storage is not supported for the current target">;
 
 def warn_maybe_falloff_nonvoid_function : Warning<
-  "control may reach end of non-void function">,
+  "not all control paths in this function return a value; non-void function must return a value">,
   InGroup;
 def warn_falloff_nonvoid_function : Warning<
-  "control reaches end of non-void function">,
+  "function does not return a value; non-void function must return a value">,
   InGroup;
 def err_maybe_falloff_nonvoid_block : Error<
   "control may reach end of non-void block">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

I will update tests after we decide on final text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits