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

[ZarkoCA]

Summary

Rewording of comments to avoid using sanity test, sanity check.

[aaron.ballman]

LGTM, thank you!

[martong]

Do we have a comprehensive list of non-inclusive terms and their inclusive correspondent somewhere available?
I mean master -> main, white list -> inclusive list, sanity -> validation, ...
I'd assume that we go through that list, and that could give me a clue of how many such patches to expect in the future.

Also, I was wondering that in list perhaps we could provide why we consider a term non-inclusive. Maybe it is just me but why is sanity considered non-inclusive?

[ZarkoCA]

In D114025#3140161, @martong wrote:

Do we have a comprehensive list of non-inclusive terms and their inclusive correspondent somewhere available?
I mean master -> main, white list -> inclusive list, sanity -> validation, ...
I'd assume that we go through that list, and that could give me a clue of how many such patches to expect in the future.

Also, I was wondering that in list perhaps we could provide why we consider a term non-inclusive. Maybe it is just me but why is sanity considered non-inclusive?

I don't know of an official LLVM list, there could be one but I am not certain.

These two sources have been a good place to start for me:
https://tools.ietf.org/id/draft-knodel-terminology-00.html
https://developers.google.com/style/inclusive-documentation

I have been working on finding replacements for blacklist, whitelist, and sanity in Clang/LLVM and @quinnp has been working on replacing master when possible. That's the extent of our list.

[• Quuxplusone]

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.

clang/lib/Sema/SemaDeclCXX.cpp

9175–9176

This comment seems incorrectly translated.

12260

This comment seems incorrectly translated.

clang/lib/StaticAnalyzer/Core/Store.cpp

252–255

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

[lebedev.ri]

In D114025#3140161, @martong wrote:

Do we have a comprehensive list of non-inclusive terms and their inclusive correspondent somewhere available?
I mean master -> main, white list -> inclusive list, sanity -> validation, ...
I'd assume that we go through that list, and that could give me a clue of how many such patches to expect in the future.

Also, I was wondering that in list perhaps we could provide why we consider a term non-inclusive. Maybe it is just me but why is sanity considered non-inclusive?

How dare you question our world's new overlords.

[aaron.ballman]

In D114025#3140161, @martong wrote:

Do we have a comprehensive list of non-inclusive terms and their inclusive correspondent somewhere available?
I mean master -> main, white list -> inclusive list, sanity -> validation, ...
I'd assume that we go through that list, and that could give me a clue of how many such patches to expect in the future.

Also, I was wondering that in list perhaps we could provide why we consider a term non-inclusive. Maybe it is just me but why is sanity considered non-inclusive?

https://gist.github.com/seanmhanson/fe370c2d8bd2b3228680e38899baf5cc has a pretty reasonable explanation about why sanity is problematic.

In D114025#3140204, @lebedev.ri wrote:

How dare you question our world's new overlords.

This is not a particularly constructive comment...

[ZarkoCA]

• Reword comments based on suggestions.

[ZarkoCA]

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

[• Quuxplusone]

clang/include/clang/AST/Redeclarable.h

261–262

// Make sure we don't infinite-loop on an invalid redecl chain. This should never happen.

clang/include/clang/CodeGen/CGFunctionInfo.h

253

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.

clang/include/clang/Sema/Lookup.h

708–709

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

clang/lib/Analysis/BodyFarm.cpp

793–795

// We expect that the property is...
although again it's not clear to me why this isn't an assertion.

clang/lib/Basic/SourceManager.cpp

61–67

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();
}

864–865

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

clang/lib/Driver/ToolChains/Clang.cpp

4305–4306

This comment can just be eliminated.

clang/lib/Sema/SemaChecking.cpp

5535–5536

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.

clang/lib/Sema/SemaDecl.cpp

12624–12626

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.

clang/lib/Sema/SemaDeclCXX.cpp

9175–9176

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.

clang/lib/Sema/SemaExpr.cpp

11187–11188

// Reject obviously incorrect shift values.
or just eliminate the comment because it's redundant with line 11189

clang/lib/Sema/SemaExprCXX.cpp

1510–1513

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.

clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp

185

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.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1672–1673

// At this point, the negation of the constraint should be infeasible. If it is feasible,...

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

329–333

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

[ZarkoCA]

• Address review comments
• Also rename some variables

[ZarkoCA]

@Quuxplusone Thanks for thorough review.

[keryell]

In D114025#3140192, @Quuxplusone wrote:

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,

It seems difficult considering the potential atmospheric pollution, carbon footprint, health issues, lung cancer, drug abuse, etc. implied.

[aaron.ballman]

In D114025#3141414, @keryell wrote:

In D114025#3140192, @Quuxplusone wrote:

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,

It seems difficult considering the potential atmospheric pollution, carbon footprint, health issues, lung cancer, drug abuse, etc. implied.

This is not a constructive comment either, please stop.

In D114025#3141358, @ZarkoCA wrote:

@Quuxplusone Thanks for thorough review.

+1, you caught some stuff I was glossing over, but this is much improved. I made a few tiny suggestions (take them or leave them). Continues to LGTM

clang/include/clang/AST/Redeclarable.h

261–262

Alternatively, "Make sure we don't infinitely loop..."

clang/lib/Sema/SemaChecking.cpp

5536

5578

[ZarkoCA]

In D114025#3142565, @aaron.ballman wrote:

In D114025#3141414, @keryell wrote:

In D114025#3140192, @Quuxplusone wrote:

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,

It seems difficult considering the potential atmospheric pollution, carbon footprint, health issues, lung cancer, drug abuse, etc. implied.

This is not a constructive comment either, please stop.

In D114025#3141358, @ZarkoCA wrote:

@Quuxplusone Thanks for thorough review.

+1, you caught some stuff I was glossing over, but this is much improved. I made a few tiny suggestions (take them or leave them). Continues to LGTM

Yes, agreed, the suggestions made this much better. Thanks.

[ZarkoCA]

• Add FIXME to comments and fix grammar on one comment

[• Quuxplusone]

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

clang/include/clang/AST/Redeclarable.h

261–262

s/on on/on an/
Also, I think comments should always end with a period ., or is it the other way around? :)

clang/include/clang/Sema/Lookup.h

708–709

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

clang/lib/Basic/DiagnosticIDs.cpp

695

Minimum, and also, I think it's a maximum? :P

clang/lib/Sema/SemaDeclCXX.cpp

9175–9176

"Don't check ... but check it anyway"?

[ZarkoCA]

• Address latest round of comments
• Fix unrelated formatting to stop distracting clang-format linter complaints

[ZarkoCA]

clang/lib/Sema/SemaDeclCXX.cpp

9175–9176

Right, that didn't make sense :). I noticed that there were warnings for this case in SemaDecl.cpp AFAIU so edited the comment to state that. Should be better now?

[• Quuxplusone]

clang/lib/Sema/SemaDeclCXX.cpp

9175–9176

Well, this version of the comment now gives the impression that it must be clear to someone. ;) I don't understand its implications, but I also don't know the code.

Specifically, I don't know what "This" refers to (grammatically) — "Anonymous union types aren't conforming, but we support them anyway, and this is the right thing to do with them"? "Our behavior in this case isn't conforming, but it wouldn't make sense to do the conforming thing [wat]"? or something else?

More fundamentally to my confusion (but not to that hypothetical other someone), I don't know what "the implicit member of the anonymous union type" actually means (in terms of the arcane details of C++), i.e., I personally don't know when this codepath is hit or what its effect is when it does get hit.

12260

Perhaps just // where they may be used by two-phase lookup. (But this is now just a nit.)

[ZarkoCA]

Thanks @aaron.ballman and @Quuxplusone for the constructive reviews.

[gandhi21299]

clang/lib/Sema/SemaChecking.cpp

5536

Is this change really necessary? It is a confusing comment and probably not too helpful for the developers.

[rjmccall]

If you aren't sure what a comment means, please feel free to CC Richard or me, and we might be able to help.

clang/include/clang/Analysis/CFG.h

520

Proper translation here is probably "assertions". A "validity check" sounds like a semantic restriction on the source language.

clang/lib/Sema/SemaChecking.cpp

5536

Perhaps "but we do to help catch trivial type errors."

5578

We enforce this for consistency with other atomics, which generally all require a trivially-copyable type because atomics just copy bits.

clang/lib/Sema/SemaDeclCXX.cpp

9175–9176

@rsmith probably has a better idea. I think it's saying that we shouldn't fall down into the generic path for the implicit field created for an anonymous union, presumably because we do special-case checks on the members of the anonymous union just above this point. I guess this isn't explicitly sanctioned by the standard but is the only sensible approach.

12260

This comment seems to be missing something — I think the previous comment was implying that the language rule really needs tags to be hidden when instantiating a using declaration. I don't know why that would be true, though; @rsmith might.

clang/lib/Sema/SemaExprCXX.cpp

1510–1513

I would prefer something like:

The standard doesn't explicitly forbid function types here, but that's an
obvious oversight, as there's no way to dynamically construct a function
in general.

[tstellar]

@ZarkoCA If you are planning to do a lot of this, it might be good to write a script (or a clang-tidy check even) that we can add to CI, so these terms don't get re-introduced.

[ZarkoCA]

@rjmccall thanks for having a look, I will clarify the comments with your suggestions.

In D114025#3152900, @tstellar wrote:

@ZarkoCA If you are planning to do a lot of this, it might be good to write a script (or a clang-tidy check even) that we can add to CI, so these terms don't get re-introduced.

That's a good idea, I will look into this definitely.

[ZarkoCA]

@rjmccall These are the proposed changes which address some of your comments. I am planning on waiting for further clarification on some of the others.

clang/include/clang/Analysis/CFG.h

520

Changed to:

/// Number of different kinds, for assertions. We subtract 1 so that
/// to keep receiving compiler warnings when we don't cover all enum values
/// in a switch

clang/lib/Sema/SemaChecking.cpp

5536

Changed to:

// GCC does not enforce these rules for GNU atomics, but we do to help catch
// trivial type errors.

5578

Changed to:

// For GNU atomics, require a trivially-copyable type. This is not part of
// the GNU atomics specification but we enforce it for consistency with
// other atomics which generally all require a trivially-copyable type. This
// is because atomics just copy bits.

clang/lib/Sema/SemaExprCXX.cpp

1510–1513

Changed to:

// The standard doesn't explicitly forbid function types here, but that's an
// obvious oversight, as there's no way to dynamically construct a function
// in general.

[rjmccall]

Those all look good, thanks.

[ZarkoCA]

In D114025#3162140, @rjmccall wrote:

Those all look good, thanks.

Committed here

[• galapoz1996]

This comment was removed by mehdi_amini.

clang/include/clang/AST/Redeclarable.h

public:
     redecl_iterator() = default;
     explicit redecl_iterator(decl_type *C) : Current(C), Starter(C) {}
 
     reference operator*() const { return Current; }
     pointer operator->() const { return Current; }
 
     redecl_iterator& operator++() {
       assert(Current && "Advancing while iterator has reached end");
-      // Sanity check to avoid infinite loop on invalid redecl chain.
+      // Make sure we don't infinitely loop on an invalid redecl chain. This
+      // should never happen.
       if (Current->isFirstDecl()) {
         if (PassedFirst) {
           assert(0 && "Passed first decl twice, invalid redecl chain!");
           Current = nullptr;
           return *this;
         }
         PassedFirst = true;
       }

clang/include/clang/Analysis/CFG.h

enum Kind {
     /// terminator statement is the same statement that branches control flow
     /// in evaluation of matching full expression.
     TemporaryDtorsBranch,
     /// A shortcut around virtual base initializers. It gets taken when
     /// virtual base classes have already been initialized by the constructor
     /// of the most derived class while we're in the base class.
     VirtualBaseBranch,
 
-    /// Number of different kinds, for sanity checks. We subtract 1 so that
+    /// Number of different kinds, for validity checks. We subtract 1 so that
     /// to keep receiving compiler warnings when we don't cover all enum values
     /// in a switch.
     NumKindsMinusOne = VirtualBaseBranch
   };
 
 private:
   static constexpr int KindBits = 2;
   static_assert((1 << KindBits) > NumKindsMinusOne,
                 "Not enough room for kind!");
   llvm::PointerIntPair<Stmt *, KindBits> Data;

clang/include/clang/CodeGen/CGFunctionInfo.h

public:
   }
 
   /// \param unpaddedCoerceToType The coerce-to type with padding elements
   ///   removed, canonicalized to a single element if it would otherwise
   ///   have exactly one element.
   static ABIArgInfo getCoerceAndExpand(llvm::StructType *coerceToType,
                                        llvm::Type *unpaddedCoerceToType) {
 #ifndef NDEBUG
-    // Sanity checks on unpaddedCoerceToType.
+    // Check that unpaddedCoerceToType has roughly the right shape.
 
     // Assert that we only have a struct type if there are multiple elements.
     auto unpaddedStruct = dyn_cast<llvm::StructType>(unpaddedCoerceToType);
     assert(!unpaddedStruct || unpaddedStruct->getNumElements() != 1);
 
     // Assert that all the non-padding elements have a corresponding element
     // in the unpadded type.
     unsigned unpaddedIndex = 0;

clang/include/clang/Sema/Lookup.h

bool isOverloadedResult() const {
     return getResultKind() == FoundOverloaded;
   }
 
   bool isUnresolvableResult() const {
     return getResultKind() == FoundUnresolvedValue;
   }
 
   LookupResultKind getResultKind() const {
-    assert(sanity());
+    assert(checkDebugAssumptions());
     return ResultKind;
   }
 
   AmbiguityKind getAmbiguityKind() const {
     assert(isAmbiguous());
     return Ambiguity;
   }
 
private:
 
   void setAmbiguous(AmbiguityKind AK) {
     ResultKind = Ambiguous;
     Ambiguity = AK;
   }
 
   void addDeclsFromBasePaths(const CXXBasePaths &P);
   void configure();
 
-  // Sanity checks.
-  bool sanity() const;
+  bool checkDebugAssumptions() const;
 
-  bool sanityCheckUnresolved() const {
+  bool checkUnresolved() const {
     for (iterator I = begin(), E = end(); I != E; ++I)
       if (isa<UnresolvedUsingValueDecl>((*I)->getUnderlyingDecl()))
         return true;
     return false;
   }
 
   static void deletePaths(CXXBasePaths *);
 

clang/lib/Analysis/BodyFarm.cpp

for (const auto *I : ImplDecl->property_impls()) {
 
       if (I->getGetterCXXConstructor()) {
         ASTMaker M(Ctx);
         return M.makeReturn(I->getGetterCXXConstructor());
       }
     }
   }
 
-  // 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.
+  // We expect 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(),
                                   Prop->getType().getNonReferenceType()))
     return nullptr;
   if (!IVar->getType()->isObjCLifetimeType() &&
       !IVar->getType().isTriviallyCopyableType(Ctx))
     return nullptr;
 
   // Generate our body:
   //   return self->_ivar;

clang/lib/Analysis/RetainSummaryManager.cpp

 
 const RetainSummary *
 RetainSummaryManager::getUnarySummary(const FunctionType* FT,
                                       ArgEffectKind AE) {
 
   // Unary functions have no arg effects by definition.
   ArgEffects ScratchArgs(AF.getEmptyMap());
 
-  // Sanity check that this is *really* a unary function.  This can
+  // Verify that this is *really* a unary function.  This can
   // happen if people do weird things.
   const FunctionProtoType* FTP = dyn_cast<FunctionProtoType>(FT);
   if (!FTP || FTP->getNumParams() != 1)
     return getPersistentStopSummary();
 
   ArgEffect Effect(AE, ObjKind::CF);
 
   ScratchArgs = AF.add(ScratchArgs, 0, Effect);

clang/lib/Basic/DiagnosticIDs.cpp

void DiagnosticIDs::getAllDiagnostics(diag::Flavor Flavor,
   for (unsigned i = 0; i != StaticDiagInfoSize; ++i)
     if (StaticDiagInfo[i].getFlavor() == Flavor)
       Diags.push_back(StaticDiagInfo[i].DiagID);
 }
 
 StringRef DiagnosticIDs::getNearestOption(diag::Flavor Flavor,
                                           StringRef Group) {
   StringRef Best;
-  unsigned BestDistance = Group.size() + 1; // Sanity threshold.
+  unsigned BestDistance = Group.size() + 1; // Maximum threshold.
   for (const WarningOption &O : OptionTable) {
     // Don't suggest ignored warning flags.
     if (!O.Members && !O.SubGroups)
       continue;
 
     unsigned Distance = O.getName().edit_distance(Group, true, BestDistance);
     if (Distance > BestDistance)
       continue;

clang/lib/Basic/SourceManager.cpp

 /// getSizeBytesMapped - Returns the number of bytes actually mapped for this
 /// ContentCache. This can be 0 if the MemBuffer was not actually expanded.
 unsigned ContentCache::getSizeBytesMapped() const {
   return Buffer ? Buffer->getBufferSize() : 0;
 }
 
 /// Returns the kind of memory used to back the memory buffer for
 /// this content cache.  This is used for performance analysis.
 llvm::MemoryBuffer::BufferKind ContentCache::getMemoryBufferKind() const {
-  assert(Buffer);
-
-  // Should be unreachable, but keep for sanity.
-  if (!Buffer)
+  if (Buffer == nullptr) {
+    assert(0 && "Buffer should never be null");
     return llvm::MemoryBuffer::MemoryBuffer_Malloc;
-
+  }
   return Buffer->getBufferKind();
 }
 
 /// getSize - Returns the size of the content encapsulated by this ContentCache.
 ///  This can be the size of the source file or the size of an arbitrary
 ///  scratch buffer.  If the ContentCache encapsulates a source file, that
 ///  file is not lazily brought in from disk to satisfy this query.
 unsigned ContentCache::getSize() const {
   return Buffer ? (unsigned)Buffer->getBufferSize()
                 : (unsigned)ContentsEntry->getSize();
while (true) {
     LessIndex = MiddleIndex;
   }
 }
 
 /// Return the FileID for a SourceLocation with a high offset.
 ///
 /// This function knows that the SourceLocation is in a loaded buffer, not a
 /// local one.
 FileID SourceManager::getFileIDLoaded(SourceLocation::UIntTy SLocOffset) const {
-  // Sanity checking, otherwise a bug may lead to hanging in release build.
   if (SLocOffset < CurrentLoadedOffset) {
     assert(0 && "Invalid SLocOffset or bad function choice");
     return FileID();
   }
 
   // Essentially the same as the local case, but the loaded array is sorted
   // in the other direction.
 
   // First do a linear scan from the last lookup position, if possible.
while (true) {
     unsigned MiddleIndex = (LessIndex - GreaterIndex) / 2 + GreaterIndex;
     const SrcMgr::SLocEntry &E = getLoadedSLocEntry(MiddleIndex);
     if (E.getOffset() == 0)
       return FileID(); // invalid entry.
 
     ++NumProbes;
 
     if (E.getOffset() > SLocOffset) {
-      // Sanity checking, otherwise a bug may lead to hanging in release build.
       if (GreaterIndex == MiddleIndex) {
         assert(0 && "binary search missed the entry");
         return FileID();
       }
       GreaterIndex = MiddleIndex;
       continue;
     }
 
     if (isOffsetInFileID(FileID::get(-int(MiddleIndex) - 2), SLocOffset)) {
       FileID Res = FileID::get(-int(MiddleIndex) - 2);
       LastFileIDLookup = Res;
       NumBinaryProbes += NumProbes;
       return Res;
     }
 
-    // Sanity checking, otherwise a bug may lead to hanging in release build.
     if (LessIndex == MiddleIndex) {
       assert(0 && "binary search missed the entry");
       return FileID();
     }
     LessIndex = MiddleIndex;
   }
 }
 

clang/lib/Driver/ToolChains/Clang.cpp

void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   const llvm::Triple &RawTriple = TC.getTriple();
   const llvm::Triple &Triple = TC.getEffectiveTriple();
   const std::string &TripleStr = Triple.getTriple();
 
   bool KernelOrKext =
       Args.hasArg(options::OPT_mkernel, options::OPT_fapple_kext);
   const Driver &D = TC.getDriver();
   ArgStringList CmdArgs;
 
-  // Check number of inputs for sanity. We need at least one input.
   assert(Inputs.size() >= 1 && "Must have at least one input.");
   // CUDA/HIP compilation may have multiple inputs (source file + results of
   // device-side compilations). OpenMP device jobs also take the host IR as a
   // second input. Module precompilation accepts a list of header files to
   // include as part of the module. All other jobs are expected to have exactly
   // one input.
   bool IsCuda = JA.isOffloading(Action::OFK_Cuda);
   bool IsCudaDevice = JA.isDeviceOffloading(Action::OFK_Cuda);
   bool IsHIP = JA.isOffloading(Action::OFK_HIP);

clang/lib/Format/Format.cpp

tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
   SmallVector<StringRef, 4> Matches;
   SmallVector<IncludeDirective, 16> IncludesInBlock;
 
   // In compiled files, consider the first #include to be the main #include of
   // the file if it is not a system #include. This ensures that the header
   // doesn't have hidden dependencies
   // (http://llvm.org/docs/CodingStandards.html#include-style).
   //
-  // FIXME: Do some sanity checking, e.g. edit distance of the base name, to fix
+  // FIXME: Do some validation, e.g. edit distance of the base name, to fix
   // cases where the first #include is unlikely to be the main header.
   tooling::IncludeCategoryManager Categories(Style.IncludeStyle, FileName);
   bool FirstIncludeBlock = true;
   bool MainIncludeFound = false;
   bool FormattingOff = false;
 
   for (;;) {
     auto Pos = Code.find('\n', SearchFrom);

clang/lib/Frontend/FrontendActions.cpp

if (llvm::Triple(LLVM_HOST_TRIPLE).isOSWindows()) {
     const SourceManager &SM = CI.getSourceManager();
     if (llvm::Optional<llvm::MemoryBufferRef> Buffer =
             SM.getBufferOrNone(SM.getMainFileID())) {
       const char *cur = Buffer->getBufferStart();
       const char *end = Buffer->getBufferEnd();
       const char *next = (cur != end) ? cur + 1 : end;
 
       // Limit ourselves to only scanning 256 characters into the source
-      // file.  This is mostly a sanity check in case the file has no
+      // file.  This is mostly a check in case the file has no
       // newlines whatsoever.
       if (end - cur > 256)
         end = cur + 256;
 
       while (next < end) {
         if (*cur == 0x0D) {  // CR
           if (*next == 0x0A) // CRLF
             BinaryMode = false;

clang/lib/Sema/SemaChecking.cpp

if (ValType.isConstQualified()) {
       Diag(ExprRange.getBegin(), diag::err_atomic_op_needs_non_const_pointer)
           << Ptr->getType() << Ptr->getSourceRange();
       return ExprError();
     }
   }
 
   // For an arithmetic operation, the implied arithmetic must be well-formed.
   if (Form == Arithmetic) {
-    // gcc does not enforce these rules for GNU atomics, but we do so for
-    // sanity.
+    // GCC does not enforce these rules for GNU atomics, but we do, because if
+    // we didn't it would be very confusing. FIXME:  For whom? How so?
     auto IsAllowedValueType = [&](QualType ValType) {
       if (ValType->isIntegerType())
         return true;
       if (ValType->isPointerType())
         return true;
       if (!ValType->isFloatingType())
         return false;
       // LLVM Parser does not allow atomicrmw with x86_fp80 type.
if (Form == Arithmetic) {
     Diag(ExprRange.getBegin(), diag::err_atomic_op_needs_atomic_int_or_ptr)
         << IsC11 << Ptr->getType() << Ptr->getSourceRange();
     return ExprError();
   }
 
   if (!IsC11 && !AtomTy.isTriviallyCopyableType(Context) &&
       !AtomTy->isScalarType()) {
     // For GNU atomics, require a trivially-copyable type. This is not part of
-    // the GNU atomics specification, but we enforce it for sanity.
+    // the GNU atomics specification, but we enforce it, because if we didn't it
+    // would be very confusing. FIXME:  For whom? How so?
     Diag(ExprRange.getBegin(), diag::err_atomic_op_needs_trivial_copy)
         << Ptr->getType() << Ptr->getSourceRange();
     return ExprError();
   }
 
   switch (ValType.getObjCLifetime()) {
   case Qualifiers::OCL_None:
   case Qualifiers::OCL_ExplicitNone:

clang/lib/Sema/SemaDecl.cpp

void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
   if (LangOpts.OpenMP &&
       (LangOpts.OpenMPIsDevice || !LangOpts.OMPTargetTriples.empty()) &&
       VDecl->isFileVarDecl())
     DeclsToCheckForDeferredDiags.insert(VDecl);
   CheckCompleteVariableDeclaration(VDecl);
 }
 
 /// ActOnInitializerError - Given that there was an error parsing an
-/// initializer for the given declaration, try to return to some form
-/// of sanity.
+/// initializer for the given declaration, try to at least re-establish
+/// invariants such as whether a variable's type is either dependent or
+/// complete.
 void Sema::ActOnInitializerError(Decl *D) {
   // Our main concern here is re-establishing invariants like "a
   // variable's type is either dependent or complete".
   if (!D || D->isInvalidDecl()) return;
 
   VarDecl *VD = dyn_cast<VarDecl>(D);
   if (!VD) return;
 
if (Previous.empty() && SkipBody && SkipBody->Previous)
     Previous.addDecl(SkipBody->Previous);
 
   if (!Previous.empty()) {
     NamedDecl *PrevDecl = Previous.getFoundDecl();
     NamedDecl *DirectPrevDecl = Previous.getRepresentativeDecl();
 
     // It's okay to have a tag decl in the same scope as a typedef
     // which hides a tag decl in the same scope.  Finding this
-    // insanity with a redeclaration lookup can only actually happen
-    // in C++.
+    // with a redeclaration lookup can only actually happen in C++.
     //
     // This is also okay for elaborated-type-specifiers, which is
     // technically forbidden by the current standard but which is
     // okay according to the likely resolution of an open issue;
     // see http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#407
     if (getLangOpts().CPlusPlus) {
       if (TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(PrevDecl)) {
         if (const TagType *TT = TD->getUnderlyingType()->getAs<TagType>()) {

clang/lib/Sema/SemaDeclCXX.cpp

if (!inUnion() && FieldRecord->isUnion() &&
         if (Diagnose)
           S.Diag(FieldRecord->getLocation(),
                  diag::note_deleted_default_ctor_all_const)
             << !!ICI << MD->getParent() << /*anonymous union*/1;
         return true;
       }
 
       // 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 supported, and we have a
+      // diagnostic for this elsewhere.
       return false;
     }
 
     if (shouldDeleteForClassSubobject(FieldRecord, FD,
                                       FieldType.getCVRQualifiers()))
       return true;
   }
 
if (RequireCompleteDeclContext(SS, LookupContext))
     return BuildInvalid();
 
   // Look up the target name.
   LookupResult R(*this, NameInfo, LookupOrdinaryName);
 
   // Unlike most lookups, we don't always want to hide tag
   // declarations: tag names are visible through the using declaration
   // even if hidden by ordinary names, *except* in a dependent context
-  // where it's important for the sanity of two-phase lookup.
+  // where they may be used by two-phase lookup.
   if (!IsInstantiation)
     R.setHideTags(false);
 
   // For the purposes of this lookup, we have a base object type
   // equal to that of the current context.
   if (CurContext->isRecord()) {
     R.setBaseObjectType(
                    Context.getTypeDeclType(cast<CXXRecordDecl>(CurContext)));

clang/lib/Sema/SemaExpr.cpp

if ((!LHSType->isFixedPointOrIntegerType() &&
       !RHSType->hasIntegerRepresentation())
     return InvalidOperands(Loc, LHS, RHS);
 
   // C++0x: Don't allow scoped enums. FIXME: Use something better than
   // hasIntegerRepresentation() above instead of this.
   if (isScopedEnumerationType(LHSType) ||
       isScopedEnumerationType(RHSType)) {
     return InvalidOperands(Loc, LHS, RHS);
   }
-  // Sanity-check shift operands
   DiagnoseBadShiftValues(*this, LHS, RHS, Loc, Opc, LHSType);
 
   // "The type of the result is that of the promoted left operand."
   return LHSType;
 }
 
 /// Diagnose bad pointer comparisons.
 static void diagnoseDistinctPointerComparison(Sema &S, SourceLocation Loc,
                                               ExprResult &LHS, ExprResult &RHS,

clang/lib/Sema/SemaExprCXX.cpp

Sema::BuildCXXTypeConstructExpr(TypeSourceInfo *TInfo,
   QualType ElemTy = Ty;
   if (Ty->isArrayType()) {
     if (!ListInitialization)
       return ExprError(Diag(TyBeginLoc, diag::err_value_init_for_array_type)
                          << FullRange);
     ElemTy = Context.getBaseElementType(Ty);
   }
 
-  // There doesn't seem to be an explicit rule against this but sanity demands
-  // we only construct objects with object types.
+  // Only construct objects with object types.
+  // There doesn't seem to be an explicit rule for this but functions are
+  // not objects, so they cannot take initializers.
   if (Ty->isFunctionType())
     return ExprError(Diag(TyBeginLoc, diag::err_init_for_function_type)
                        << Ty << FullRange);
 
   // C++17 [expr.type.conv]p2:
   //   If the type is cv void and the initializer is (), the expression is a
   //   prvalue of the specified type that performs no initialization.
   if (!Ty->isVoidType() &&
       RequireCompleteType(TyBeginLoc, ElemTy,

clang/lib/Sema/SemaLookup.cpp

void LookupResult::configure() {
   if (IdentifierInfo *Id = NameInfo.getName().getAsIdentifierInfo()) {
     if (unsigned BuiltinID = Id->getBuiltinID()) {
       if (!getSema().Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID))
         AllowHidden = true;
     }
   }
 }
 
-bool LookupResult::sanity() const {
+bool LookupResult::checkDebugAssumptions() const {
   // This function is never called by NDEBUG builds.
   assert(ResultKind != NotFound || Decls.size() == 0);
   assert(ResultKind != Found || Decls.size() == 1);
   assert(ResultKind != FoundOverloaded || Decls.size() > 1 ||
          (Decls.size() == 1 &&
           isa<FunctionTemplateDecl>((*begin())->getUnderlyingDecl())));
-  assert(ResultKind != FoundUnresolvedValue || sanityCheckUnresolved());
+  assert(ResultKind != FoundUnresolvedValue || checkUnresolved());
   assert(ResultKind != Ambiguous || Decls.size() > 1 ||
          (Decls.size() == 1 && (Ambiguity == AmbiguousBaseSubobjects ||
                                 Ambiguity == AmbiguousBaseSubobjectTypes)));
   assert((Paths != nullptr) == (ResultKind == Ambiguous &&
                                 (Ambiguity == AmbiguousBaseSubobjectTypes ||
                                  Ambiguity == AmbiguousBaseSubobjects)));
   return true;
 }

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

void CStringChecker::evalStrcat(CheckerContext &C, const CallExpr *CE) const {
   // char *strcat(char *restrict s1, const char *restrict s2);
   evalStrcpyCommon(C, CE,
                    /* ReturnEnd = */ false,
                    /* IsBounded = */ false,
                    /* appendK = */ ConcatFnKind::strcat);
 }
 
 void CStringChecker::evalStrncat(CheckerContext &C, const CallExpr *CE) const {
-  //char *strncat(char *restrict s1, const char *restrict s2, size_t n);
+  // char *strncat(char *restrict s1, const char *restrict s2, size_t n);
   evalStrcpyCommon(C, CE,
                    /* ReturnEnd = */ false,
                    /* IsBounded = */ true,
                    /* appendK = */ ConcatFnKind::strcat);
 }
 
 void CStringChecker::evalStrlcat(CheckerContext &C, const CallExpr *CE) const {
   // size_t strlcat(char *dst, const char *src, size_t size);
void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
 
   state = state->BindExpr(CE, LCtx, resultVal);
 
   // Record this as a possible path.
   C.addTransition(state);
 }
 
 void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const {
-  //char *strsep(char **stringp, const char *delim);
-  // Sanity: does the search string parameter match the return type?
+  // char *strsep(char **stringp, const char *delim);
+  // Verify whether the search string parameter matches the return type.
   SourceArgExpr SearchStrPtr = {CE->getArg(0), 0};
 
   QualType CharPtrTy = SearchStrPtr.Expression->getType()->getPointeeType();
   if (CharPtrTy.isNull() ||
       CE->getType().getUnqualifiedType() != CharPtrTy.getUnqualifiedType())
     return;
 
   CurrentFunctionDescription = "strsep()";

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

while (LC) {
     LC = SFC->getParent();
   }
 
   const Decl *D = SFC->getDecl();
 
   if (Optional<AnyCall> AC = AnyCall::forDecl(D)) {
     // Even though there's a Sema warning when the return type of an annotated
     // function is not a kern_return_t, this warning isn't an error, so we need
-    // an extra sanity check here.
+    // an extra check here.
     // FIXME: AnyCall doesn't support blocks yet, so they remain unchecked
     // for now.
     if (!AC->getReturnType(C.getASTContext())
              .getCanonicalType()->isSignedIntegerType())
       return false;
   }
 
   if (D->hasAttr<MIGServerRoutineAttr>())

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

virtual std::string describe(ProgramStateRef State,
       // constraints, e.g. ComparisonConstraint. In that case we can safely
       // ignore the implementation of this function.
       llvm_unreachable("Not implemented");
     }
 
   protected:
     ArgNo ArgN; // Argument to which we apply the constraint.
 
-    /// Do polymorphic sanity check on the constraint.
+    /// Do polymorphic validation check on the constraint.
     virtual bool checkSpecificValidity(const FunctionDecl *FD) const {
       return true;
     }
   };
 
   /// Given a range, should the argument stay inside or outside this range?
   enum RangeKind { OutOfRange, WithinRange };
 
bool matchesAndSet(const Signature &Sign, const FunctionDecl *FD) {
       if (Result) {
         assert(!this->FD && "FD must not be set more than once");
         this->FD = FD;
       }
       return Result;
     }
 
   private:
-    // Once we know the exact type of the function then do sanity check on all
-    // the given constraints.
+    // Once we know the exact type of the function then do validation check on
+    // all the given constraints.
     bool validateByConstraints(const FunctionDecl *FD) const {
       for (const ConstraintSet &Case : CaseConstraints)
         for (const ValueConstraintPtr &Constraint : Case)
           if (!Constraint->checkValidity(FD))
             return false;
       for (const ValueConstraintPtr &Constraint : ArgConstraints)
         if (!Constraint->checkValidity(FD))
           return false;

clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp

void UnixAPIMisuseChecker::CheckOpenVariant(CheckerContext &C,
   unsigned int CreateModeArgIndex = FlagsArgIndex + 1;
 
   // The create mode argument should be the last argument.
   unsigned int MaxArgCount = CreateModeArgIndex + 1;
 
   ProgramStateRef state = C.getState();
 
   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. Just return.
     return;
   } else if (CE->getNumArgs() == MaxArgCount) {
     const Expr *Arg = CE->getArg(CreateModeArgIndex);
     QualType QT = Arg->getType();
     if (!QT->isIntegerType()) {
       SmallString<256> SBuf;
       llvm::raw_svector_ostream OS(SBuf);
       OS << "The " << CreateModeArgIndex + 1

 
 // Does a basic check for 0-sized allocations suitable for most of the below
 // functions (modulo "calloc")
 void UnixAPIPortabilityChecker::BasicAllocationCheck(CheckerContext &C,
                                                      const CallExpr *CE,
                                                      const unsigned numArgs,
                                                      const unsigned sizeArg,
                                                      const char *fn) const {
-  // Sanity check for the correct number of arguments
+  // Check for the correct number of arguments.
   if (CE->getNumArgs() != numArgs)
     return;
 
   // Check if the allocation size is 0.
   ProgramStateRef state = C.getState();
   ProgramStateRef trueState = nullptr, falseState = nullptr;
   const Expr *arg = CE->getArg(sizeArg);
   SVal argVal = C.getSVal(arg);

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

if (!isUnderconstrained(N))
       IsTrackingTurnedOn = true;
   if (!IsTrackingTurnedOn)
     return nullptr;
 
   // Check if in the previous state it was feasible for this constraint
   // to *not* be true.
   if (isUnderconstrained(PrevN)) {
     IsSatisfied = true;
 
-    // As a sanity check, make sure that the negation of the constraint
-    // was infeasible in the current state.  If it is feasible, we somehow
-    // missed the transition point.
+    // At this point, the negation of the constraint should be infeasible. If it
+    // is feasible, make sure that the negation of the constrainti was
+    // infeasible in the current state.  If it is feasible, we somehow missed
+    // the transition point.
     assert(!isUnderconstrained(N));
 
     // We found the transition point for the constraint.  We now need to
     // pretty-print the constraint. (work-in-progress)
     SmallString<64> sbuf;
     llvm::raw_svector_ostream os(sbuf);
 
     if (Constraint.getAs<Loc>()) {

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

if (!Result) {
     // mode. Only create a region if the current value is a NonLoc.
     if (!InitValWithAdjustments.getAs<NonLoc>()) {
       if (OutRegionWithAdjustments)
         *OutRegionWithAdjustments = nullptr;
       return State;
     }
     Result = InitWithAdjustments;
   } else {
-    // We need to create a region no matter what. For sanity, make sure we don't
-    // try to stuff a Loc into a non-pointer temporary region.
+    // 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());
   }
 
   ProgramStateManager &StateMgr = State->getStateManager();
   MemRegionManager &MRMgr = StateMgr.getRegionManager();
   StoreManager &StoreMgr = StateMgr.getStoreManager();
 
   // MaterializeTemporaryExpr may appear out of place, after a few field and
   // base-class accesses have been made to the object, even though semantically

clang/lib/StaticAnalyzer/Core/Store.cpp

static bool regionMatchesCXXRecordType(SVal V, QualType Ty) {
   const CXXRecordDecl *Expected = Ty->getPointeeCXXRecordDecl();
   if (!Expected)
     Expected = Ty->getAsCXXRecordDecl();
 
   return Expected->getCanonicalDecl() == RD->getCanonicalDecl();
 }
 
 SVal StoreManager::evalDerivedToBase(SVal Derived, const CastExpr *Cast) {
-  // Sanity check to avoid doing the wrong thing in the face of
+  // Early return to avoid doing the wrong thing in the face of
   // reinterpret_cast.
   if (!regionMatchesCXXRecordType(Derived, Cast->getSubExpr()->getType()))
     return UnknownVal();
 
   // Walk through the cast path to create nested CXXBaseRegions.
   SVal Result = Derived;
   for (CastExpr::path_const_iterator I = Cast->path_begin(),
                                      E = Cast->path_end();
        I != E; ++I) {
     Result = evalDerivedToBase(Result, (*I)->getType(), (*I)->isVirtual());
   }

clang/lib/Tooling/Syntax/Tree.cpp

assert((!Begin || Begin->Parent == this) &&
          "`Begin` is not a child of `this`.");
   assert((!End || End->Parent == this) && "`End` is not a child of `this`.");
   assert(canModify() && "Cannot modify `this`.");
 
 #ifndef NDEBUG
   for (auto *N = New; N; N = N->NextSibling) {
     assert(N->Parent == nullptr);
     assert(N->getRole() != NodeRole::Detached && "Roles must be set");
-    // FIXME: sanity-check the role.
+    // FIXME: validate the role.
   }
 
   auto Reachable = [](Node *From, Node *N) {
     if (!N)
       return true;
     for (auto *It = From; It; It = It->NextSibling)
       if (It == N)
         return true;