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