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