aaron.ballman added reviewers: aaron.ballman, erichkeane, xgupta.
aaron.ballman added a comment.

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



================
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;
----------------
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.


================
Comment at: clang/test/SemaCXX/warn-infinite-recursion.cpp:11
   else
-    b(x+1);
+    b(x + 1);
 }
----------------
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.


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