erichkeane 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: > 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. 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
