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

Thu Nov 18 08:15:56 PST 2021

ZarkoCA added a comment.

In D114025#3140192 <https://reviews.llvm.org/D114025#3140192>, @Quuxplusone wrote:

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

I think I understand your point and have tried to address that. Additionally, I would also argue that `sanity check/test` also serve as buzzwords and, while we may need to take greater care of how we reword things so they convey the meaning better, it shouldn't be fine to find replacements for them. Your review helps, thanks.

> "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.

I've tried to address this in the updated diff. However, I think this also makes a case that there is a general issue with using `sanity check/test` aside from inclusive language.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025