[PATCH] D127759: [Diagnostic] Clarify -Winfinite-recursion message

[Muhammad Usman Shahid via Phabricator via cfe-commits]

Codesbyusman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:63
 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;
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > 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.
> > 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 :) 
> 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.
**"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. 


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:63
 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;
----------------
Codesbyusman wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > 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 :) 
> > 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.
> **"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. 
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.


================
Comment at: clang/test/SemaCXX/warn-infinite-recursion.cpp:11
   else
-    b(x+1);
+    b(x + 1);
 }
----------------
aaron.ballman wrote:
> 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.
my bad, didn't notice,  on save formatter was enabled. I will revert it. By the way nice advice. Thank you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127759