aaron.ballman 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;
----------------
erichkeane wrote:
> erichkeane wrote:
> > Codesbyusman wrote:
> > > 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.
> > I think I'd like the first one, the location data alone is pretty annoying 
> > in many cases.
> >>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.
> 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`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127759

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

Reply via email to