[PATCH] D114025: [clang][NFC] Inclusive terms: replace some uses of sanity in clang

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

(Re repeated thanks: You're welcome. :) For the record, I personally see nothing wrong with the phrase "sanity-check" either; but given that it's gonna happen, and we're re-wording comments on by definition the subtlest and most confusing parts of the code, I'm trying to help us not to lose/distort the semantics of those comments. Some of these comments are even //improving// through this exercise.)



================
Comment at: clang/include/clang/AST/Redeclarable.h:261
       assert(Current && "Advancing while iterator has reached end");
-      // Sanity check to avoid infinite loop on invalid redecl chain.
+      // Validation check to avoid infinite loop on invalid redecl chain.
       if (Current->isFirstDecl()) {
----------------
aaron.ballman wrote:
> Quuxplusone wrote:
> > `// Make sure we don't infinite-loop on an invalid redecl chain. This should never happen.`
> Alternatively, "Make sure we don't infinitely loop..."
s/on on/on an/
Also, I think comments should always end with a period `.`, or is it the other way around? :)


================
Comment at: clang/include/clang/Sema/Lookup.h:709-710
 
-  // Sanity checks.
+  // Validation checks.
   bool sanity() const;
 
----------------
Quuxplusone wrote:
> The original comment strikes me as pretty useless, except that it's kind of obliquely explaining the meaning of the ungrammatical `bool sanity() const`. It could have been fixed better, pre-PC, by just removing the comment and changing the function to `bool isSane() const`. I have no particular suggestion for this one, other than "you'll have to look at how it's used" and/or "just leave the comment alone, until you're ready to rename the actual identifiers too" and/or "just delete the comment because it's useless."
The new version has the problem that `check()` is really vague, which again means that in order to explain what it does, you have to use the term "sanity-check". ;)  Perhaps change the identifier to `checkAssumptions()` or even `checkDebugAssumptions()`?
(Pre-existing problem: the name of the function still doesn't describe what it does, because it doesn't //check// or //assert// anything; it simply returns true or false, and it's up to the caller to `assert` on the return value. But `conformsToAssumptions()` is a silly name.)


================
Comment at: clang/lib/Basic/DiagnosticIDs.cpp:695
   StringRef Best;
-  unsigned BestDistance = Group.size() + 1; // Sanity threshold.
+  unsigned BestDistance = Group.size() + 1; // Minumum threshold.
   for (const WarningOption &O : OptionTable) {
----------------
`Minimum`, and also, I think it's a maximum? :P


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:9176
       // Don't check the implicit member of the anonymous union type.
-      // This is technically non-conformant, but sanity demands it.
+      // This is technically non-conformant, but validation tests demand it.
       return false;
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > This comment seems incorrectly translated.
> This comment //still// seems incorrectly translated.
> Things we do "for sanity's sake" aren't necessarily //required//, technically; but we're doing them anyway, for sanity.
"Don't check ... but check it anyway"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025