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

Thu Nov 18 09:35:55 PST 2021

Quuxplusone added inline comments.

================
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()) {
----------------
`// Make sure we don't infinite-loop on an invalid redecl chain. This should never happen.`

================
Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:253
 #ifndef NDEBUG
-    // Sanity checks on unpaddedCoerceToType.
+    // Validation checks on unpaddedCoerceToType.

----------------
This still feels like a misuse of "validation."
I'd say something like `// Check that unpaddedCoerceToType has roughly the right shape.`

To me, "validation" suggests the phrase "passes validations," which is like a Quality Assurance thing, and means "we did a lot of tests and couldn't break it, it's definitely valid," which is the exact //opposite// of the rough-cut quick-and-dirty smoke-test we mean when we say "sanity check." 

After we have //validated// an input, the postcondition should be that the input is //valid//. That's not what's happening in any of these cases. These sanity-checks are just rejecting specific easy-to-detect //invalid// cases. They're not digging deep to actually validate the input. They're just sanity-checking it.

================
Comment at: clang/include/clang/Sema/Lookup.h:709-710

-  // Sanity checks.
+  // Validation checks.
   bool sanity() const;

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

================
Comment at: clang/lib/Analysis/BodyFarm.cpp:793-794

-  // Sanity check that the property is the same type as the ivar, or a
-  // reference to it, and that it is either an object pointer or trivially
-  // copyable.
+  // Verify that the property is the same type as the ivar, or a reference to
+  // it, and that it is either an object pointer or trivially copyable.
   if (!Ctx.hasSameUnqualifiedType(IVar->getType(),
----------------
`// We expect that the property is...`
although again it's not clear to me why this isn't an assertion.

================
Comment at: clang/lib/Basic/SourceManager.cpp:61-69
 llvm::MemoryBuffer::BufferKind ContentCache::getMemoryBufferKind() const {
   assert(Buffer);

-  // Should be unreachable, but keep for sanity.
+  // Should be unreachable, but keep for validation checking.
   if (!Buffer)
     return llvm::MemoryBuffer::MemoryBuffer_Malloc;

----------------
This code seems silly to me. I would just delete these lines; or else how about
```
llvm::MemoryBuffer::BufferKind ContentCache::getMemoryBufferKind() const {
  if (Buffer == nullptr) {
    assert(0 && "Buffer should never be null");
    return llvm::MemoryBuffer::MemoryBuffer_Malloc;
  }
  return Buffer->getBufferKind();
}
```

================
Comment at: clang/lib/Basic/SourceManager.cpp:867
 FileID SourceManager::getFileIDLoaded(SourceLocation::UIntTy SLocOffset) const {
-  // Sanity checking, otherwise a bug may lead to hanging in release build.
+  // Required, otherwise a bug may lead to hanging in release build.
   if (SLocOffset < CurrentLoadedOffset) {
----------------
Throughout this file, I'd just delete these comments.
This isn't the use of "sanity-checking" I'm familiar with, btw. This is what I would call "defensive programming," where we believe a codepath is unreachable but we leave it in the codebase anyway "just in case." I think the fact that the codepath starts with `assert(0 && "reason")` is sufficient to mark it as a defensive codepath, and it doesn't need any comment about why it's "sane" and/or "required".

The codepath on line 64 is the same deal.

Validation: Ensure that the state is good. (Can generally be done only to user inputs. Once pointers are in the mix, "validation" of an intermediate state quickly becomes Halting-Problem-hard.)
Sanity-checking/smoke-testing: Quickly reject some states that are obviously bad. (Assert-fail when "this should never be happening.")
Defensive programming: Do something sensible even when in a bad state. ("This should never be happening," but describe what to do //instead// of asserting, anyway.)
"For sanity": This data field should never be read ever again, but just in case it is, put something sensible into it. ("That should never happen.")

================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4306

-  // Check number of inputs for sanity. We need at least one input.
+  // Check number of inputs for validity. We need at least one input.
   assert(Inputs.size() >= 1 && "Must have at least one input.");
----------------
This comment can just be eliminated.

================
Comment at: clang/lib/Sema/SemaChecking.cpp:5536
     // gcc does not enforce these rules for GNU atomics, but we do so for
-    // sanity.
+    // validation.
     auto IsAllowedValueType = [&](QualType ValType) {
----------------
This is the "for sanity" idiom; it has nothing to do with validation or even smoke-testing. This comment translates to something like
```
// GCC does not enforce these rules for GNU atomics, but we do,
// because if we didn't, it would be very confusing. [For whom? How so?]
```
Ditto line 5577.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:12625
 /// initializer for the given declaration, try to return to some form
-/// of sanity.
+/// of validity.
 void Sema::ActOnInitializerError(Decl *D) {
----------------
I suggest that lines 12627-12628 should be moved up here and combined with this comment in some way. We have the power to be much less vague here about the precise form of sanity we're attempting to restore.

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

================
Comment at: clang/lib/Sema/SemaExpr.cpp:11188
   }
-  // Sanity-check shift operands
+  // Verify shift operands
   DiagnoseBadShiftValues(*this, LHS, RHS, Loc, Opc, LHSType);
----------------
`// Reject obviously incorrect shift values.`
or just eliminate the comment because it's redundant with line 11189

================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:1510-1511

-  // There doesn't seem to be an explicit rule against this but sanity demands
-  // we only construct objects with object types.
+  // There doesn't seem to be an explicit rule against this but validation
+  // testing demands we only construct objects with object types.
   if (Ty->isFunctionType())
----------------
This comment is incorrectly translated. I'm not sure I understand what's going on on this codepath, but it seems like the right comment is just
`// Functions aren't objects, so they can't take initializers.`
The original commenter would have added, `The Standard doesn't seem to say this anywhere, but it makes sense, right?` However, I'm sure there must be at least an open issue about this, if it hasn't already been fixed; that second sentence would just bit-rot, so I wouldn't bother adding it.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:185
   if (CE->getNumArgs() < MinArgCount) {
-    // The frontend should issue a warning for this case, so this is a sanity
-    // check.
+    // The frontend should issue a warning for this case, check for that here.
     return;
----------------
I believe this is another instance of someone misusing the term "sanity check" for (something like) "defensive programming." We're not //checking// anything on this codepath; we're doing something sensible in a case that has already been dealt with elsewhere (but in this case, it actually //can// fall through to here; asserting here would be wrong). I'd write:
`// The frontend has already issued a warning. Just return.`

================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1673

-    // As a sanity check, make sure that the negation of the constraint
+    // As a validation check, make sure that the negation of the constraint
     // was infeasible in the current state.  If it is feasible, we somehow
----------------
`// At this point, the negation of the constraint should be infeasible. If it is feasible,...`

================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:329-333
+    // We need to create a region no matter what. Make sure we don't try to
+    // stuff a Loc into a non-pointer temporary region.
     assert(!InitValWithAdjustments.getAs<Loc>() ||
            Loc::isLocType(Result->getType()) ||
            Result->getType()->isMemberPointerType());
----------------
This seems to be the "for sanity" idiom, but I have no idea what it's trying to say (because I don't know this code).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025