[Diagnostic] Clarify -Winfinite-recursion message

[aaron.ballman]

Thank you for looking into this! I'm adding a few more reviewers for awareness.

clang/include/clang/Basic/DiagnosticSemaKinds.td

63

A few things:

• Diagnostics in Clang are not intended to be grammatically correct, so they don't start with a capital letter or end with a trailing full stop (so if this wording was kept, you should use in order instead of In order).
• While this is a cute way to get the point across, we want the diagnostic to help guide the user as to why their code is wrong so they have the best chance to fix the issue.

I think the original wording isn't too bad here because it's lexically associated with the function definition itself (so the "this function" is clear in situ), but I do think "will call itself" is a bit awkward. Perhaps "this function is called recursively on all paths through the function", but I don't feel strongly either.

clang/test/SemaCXX/warn-infinite-recursion.cpp

11

It looks like you formatted the whole file -- we typically try to have the only changes in the patch be specifically about the changes driving the patch, instead of mixing several fixes together into one patch. While it seems a bit less efficient than it could be, having a clean separation eases maintenance burdens for us. If we need to revert a patch for some reason, we don't lose potentially good changes along with the bad ones. But also, it makes it more clear if we need to do a git-blame as to why changes were introduced.

You should revert the formatting-specific changes in the file so that the only differences are the functional ones.

[erichkeane]

clang/include/clang/Basic/DiagnosticSemaKinds.td

63

I like the suggestion by Aaron here, "function %0 is called recursively through all paths through %0" (or the function). Perhaps the double %0 is a touch of a recursion joke as well :)

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

63

TBH, I'd probably skip the %0 because the warning is issued on the same line as the function definition itself, so the name should already be obvious to the user.

This diagnostic doesn't currently handle mutually recursive functions, and I kind of worry that adding %0 implies we support that case when we don't.

[Codesbyusman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

63

"this function is called recursively on all paths through the function" is good. what you say which will be more better as the "all paths through this function will call itself" is also ok.

63

if i am not wrong it is indicating the infinite recursion as no base case. can we prompt user in some a decent manner that the base case is missing. i.e. including some phrase in "this function is called recursively on all paths through the function" that shows base case was missing or this would be a bad idea? any suggestions.

clang/test/SemaCXX/warn-infinite-recursion.cpp

11

my bad, didn't notice, on save formatter was enabled. I will revert it. By the way nice advice. Thank you

[erichkeane]

clang/include/clang/Basic/DiagnosticSemaKinds.td

63

I think I'd like the first one, the location data alone is pretty annoying in many cases.

[erichkeane]

clang/include/clang/Basic/DiagnosticSemaKinds.td

63

I think I'd like the first one, the location data alone is pretty annoying in many cases.

Spoke with Aaron offline, our policy is to just use the names when it is ambiguous/helpful in a way that knowing the line is not. I'm not a huge fan of the policy, but am not going to lobby it here, feel free to use one of hte below suggestions.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

63

Spoke with Aaron offline, our policy is to just use the names when it is ambiguous/helpful in a way that knowing the line is not. I'm not a huge fan of the policy, but am not going to lobby it here, feel free to use one of hte below suggestions.

FWIW, my feelings are not super strongly held (and it's possible this isn't "policy" so much as "the way some folks do things"), so if @Codesbyusman decides he wants to use %0 to name the function in the diagnostic or not, that's fine by me also. The only bit I definitely feel strongly about is not repeating the function name in the diagnostic and avoiding an implication that the diagnostic works for detecting mutual recursion. So I'd be fine with function %0 is called recursively on all paths through the function

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_max_unsigned_zero : Warning<
   "taking the max of "
   "%select{a value and unsigned zero|unsigned zero and a value}0 "
   "is always equal to the other value">,
   InGroup<MaxUnsignedZero>;
 def note_remove_max_call : Note<
   "remove call to max function and unsigned zero argument">;
 
 def warn_infinite_recursive_function : Warning<
-  "all paths through this function will call itself">,
+  "In order to understand recursion, you must first understand recursion">,
   InGroup<InfiniteRecursion>, DefaultIgnore;
 
 def warn_comma_operator : Warning<"possible misuse of comma operator here">,
   InGroup<DiagGroup<"comma">>, DefaultIgnore;
 def note_cast_to_void : Note<"cast expression to void to silence warning">;
 def note_cast_operand_to_int : Note<"cast one or both operands to int to silence this warning">;
 
 // Constant expressions

clang/test/SemaCXX/warn-infinite-recursion.cpp

 // RUN: %clang_cc1 %s -fsyntax-only -verify -Winfinite-recursion
 
-void a() {  // expected-warning{{call itself}}
+void a() { // expected-warning{{to understand recursion}}
   a();
 }
 
-void b(int x) {  // expected-warning{{call itself}}
+void b(int x) { // expected-warning{{to understand recursion}}
   if (x)
     b(x);
   else
-    b(x+1);
+    b(x + 1);
 }
 
 void c(int x) {
   if (x)
     c(5);
 }
 
-void d(int x) {  // expected-warning{{call itself}}
+void d(int x) { // expected-warning{{to understand recursion}}
   if (x)
     ++x;
   return d(x);
 }
 
 // Doesn't warn on mutually recursive functions
 void e();
 void f();
 
 void e() { f(); }
 void f() { e(); }
 
-void g() {  // expected-warning{{call itself}}
+void g() { // expected-warning{{to understand recursion}}
   while (true)
     g();
 
   g();
 }
 
 void h(int x) {
   while (x < 5) {
-    h(x+1);
+    h(x + 1);
   }
 }
 
-void i(int x) {  // expected-warning{{call itself}}
+void i(int x) { // expected-warning{{to understand recursion}}
   while (x < 5) {
     --x;
   }
   i(0);
 }
 
-int j() {  // expected-warning{{call itself}}
+int j() { // expected-warning{{to understand recursion}}
   return 5 + j();
 }
 
 // Don't warn on infinite loops
 void k() {
-  while(true) {
+  while (true) {
     k();
   }
 }
 
 void l() {
-  while (true) {}
+  while (true) {
+  }
 
   l();
 }
 
 void m() {
   static int count = 5;
-  if (count >0) {
+  if (count > 0) {
     count--;
     l();
   }
-  while (true) {}
+  while (true) {
+  }
 }
 
 class S {
   static void a();
   void b();
 };
 
-void S::a() {  // expected-warning{{call itself}}
+void S::a() { // expected-warning{{to understand recursion}}
   return a();
 }
 
-void S::b() {  // expected-warning{{call itself}}
+void S::b() { // expected-warning{{to understand recursion}}
   int i = 0;
   do {
     ++i;
     b();
   } while (i > 5);
 }
 
-template<class member>
+template <class member>
 struct T {
   member m;
-  void a() { return a(); }  // expected-warning{{call itself}}
-  static void b() { return b(); }  // expected-warning{{call itself}}
+  void a() { return a(); }        // expected-warning{{to understand recursion}}
+  static void b() { return b(); } // expected-warning{{to understand recursion}}
 };
 
 void test_T() {
   T<int> foo;
-  foo.a();  // expected-note{{in instantiation}}
-  foo.b();  // expected-note{{in instantiation}}
+  foo.a(); // expected-note{{in instantiation}}
+  foo.b(); // expected-note{{in instantiation}}
 }
 
 class U {
-  U* u;
-  void Fun() {  // expected-warning{{call itself}}
+  U *u;
+  void Fun() { // expected-warning{{to understand recursion}}
     u->Fun();
   }
 };
 
 // No warnings on templated functions
-// sum<0>() is instantiated, does recursively call itself, but never runs.
+// sum<0>() is instantiated, does recursively to understand recursion, but never runs.
 template <int value>
 int sum() {
-  return value + sum<value/2>();
+  return value + sum<value / 2>();
 }
 
-template<>
+template <>
 int sum<1>() { return 1; }
 
-template<int x, int y>
+template <int x, int y>
 int calculate_value() {
   if (x != y)
-    return sum<x - y>();  // This instantiates sum<0>() even if never called.
+    return sum<x - y>(); // This instantiates sum<0>() even if never called.
   else
     return 0;
 }
 
-int value = calculate_value<1,1>();
+int value = calculate_value<1, 1>();
 
 void DoSomethingHere();
 
 // DoStuff<0,0>() is instantiated, but never called.
-template<int First, int Last>
+template <int First, int Last>
 int DoStuff() {
   if (First + 1 == Last) {
     // This branch gets removed during <0, 0> instantiation in so CFG for this
     // function goes straight to the else branch.
     DoSomethingHere();
   } else {
-    DoStuff<First, (First + Last)/2>();
-    DoStuff<(First + Last)/2, Last>();
+    DoStuff<First, (First + Last) / 2>();
+    DoStuff<(First + Last) / 2, Last>();
   }
   return 0;
 }
 int stuff = DoStuff<0, 1>();
 
-template<int x>
+template <int x>
 struct Wrapper {
   static int run() {
     // Similar to the above, Wrapper<0>::run() will discard the if statement.
     if (x == 1)
       return 0;
-    return Wrapper<x/2>::run();
+    return Wrapper<x / 2>::run();
   }
-  static int run2() {  // expected-warning{{call itself}}
+  static int run2() { // expected-warning{{to understand recursion}}
     return run2();
   }
 };
 
 template <int x>
 int test_wrapper() {
   if (x != 0)
     return Wrapper<x>::run() +
-           Wrapper<x>::run2();  // expected-note{{instantiation}}
+           Wrapper<x>::run2(); // expected-note{{instantiation}}
   return 0;
 }
 
-int wrapper_sum = test_wrapper<2>();  // expected-note{{instantiation}}
+int wrapper_sum = test_wrapper<2>(); // expected-note{{instantiation}}