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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
