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