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

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Peanut gallery says: I'd recommend //not// taking this particular patch; it seems much less "obvious" than the whitelist/inclusionlist type of patch. Whereas "whitelist" is just a made-up buzzword that can easily be replaced with a different made-up buzzword, "sanity check" is not at all the same as "validation test." In many cases, I think "sanity-check" could be reasonably replaced with "smoke-test," but (1) this PR doesn't do that, and (2) the phrase "smoke-test" is probably //harder// to understand, and (3) this PR currently touches many instances of "sanity/insanity/sane/insane" in code comments which have nothing to do with testing or checking at all.

"We could do X, but that would be insane" does //not// mean "We could do X, but that would not pass validations." It means it would be //stupid//, //irrational//, //foolish for obvious reasons//, something along those lines.

I agree that "X is dumb/stupid/insane" is a bad code comment and should be improved. It is bad //not just// because it is un-PC but because it is vague and therefore not (directly) helpful to the reader. However, you cannot fix this kind of comment by just substituting some made-up reason like "it would fail validation," because a //lying// comment is //worse// than a vague comment.

Not all of the individual diffs in this PR are bad. But some of them are.



================
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;
----------------
This comment seems incorrectly translated.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:12260
   // even if hidden by ordinary names, *except* in a dependent context
-  // where it's important for the sanity of two-phase lookup.
+  // where it's important for the validation of two-phase lookup.
   if (!IsInstantiation)
----------------
This comment seems incorrectly translated.


================
Comment at: clang/lib/StaticAnalyzer/Core/Store.cpp:252-255
+  // Verify that we avoid doing the wrong thing in the face of
   // reinterpret_cast.
   if (!regionMatchesCXXRecordType(Derived, Cast->getSubExpr()->getType()))
     return UnknownVal();
----------------
This comment seems incorrectly translated. I interpret the writer's intent as, "We really don't ever expect to get here from a reinterpret_cast; but if we ever do, let's just return something plausible [so that we avoid doing anything insane in the following codepath]."

According to my interpretation, this would be better expressed as a simple `assert`, and you should file a bug if the assert ever gets hit.

Alternatively, this `if` is necessary for correctness, and the comment should simply say something like "We can get here from a reinterpret_cast; in that case, return UnknownVal so that [whatever the reason is]."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025