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

[Gaven Rank via Phabricator via cfe-commits]

galapoz1996 added a comment.
Herald added subscribers: steakhal, MaskRay.
Herald added a reviewer: NoQ.
Herald added a project: All.

I used to think that therapy was only for people with "real" problems, but I 
couldn't have been more wrong. Seeking help for my anxiety and depression has 
been one of the best decisions I've ever made us.calmerry.com 
 . If you're struggling, don't hesitate to reach out 
for help - there are plenty of resources online to help you find a therapist 
that's right for you.


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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA added a comment.

In D114025#3162140 , @rjmccall wrote:

> Those all look good, thanks.

Committed here 



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

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Those all look good, thanks.


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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA added a comment.

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




Comment at: clang/include/clang/Analysis/CFG.h:520
 /// to keep receiving compiler warnings when we don't cover all enum values
 /// in a switch.
 NumKindsMinusOne = VirtualBaseBranch

rjmccall wrote:
> Proper translation here is probably "assertions".   A "validity check" sounds 
> like a semantic restriction on the source language.
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
```



Comment at: clang/lib/Sema/SemaChecking.cpp:5536
+// 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?]
 auto IsAllowedValueType = [&](QualType ValType) {

rjmccall wrote:
> aaron.ballman wrote:
> > 
> Perhaps "but we do to help catch trivial type errors."
Changed to: 
```

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



Comment at: clang/lib/Sema/SemaChecking.cpp:5578
+// the GNU atomics specification, but we enforce it, because if we didn't 
it
+// would be very confusing [For whom? How so?]
 Diag(ExprRange.getBegin(), diag::err_atomic_op_needs_trivial_copy)

rjmccall wrote:
> aaron.ballman wrote:
> > 
> We enforce this for consistency with other atomics, which generally all 
> require a trivially-copyable type because atomics just copy bits.
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.
```



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

rjmccall wrote:
> Quuxplusone wrote:
> > 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.
> 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.
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.
```



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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA added a comment.

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


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

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

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

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


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

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

[John McCall via Phabricator via cfe-commits]

rjmccall added subscribers: rsmith, rjmccall.
rjmccall added a comment.

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




Comment at: clang/include/clang/Analysis/CFG.h:520
 /// to keep receiving compiler warnings when we don't cover all enum values
 /// in a switch.
 NumKindsMinusOne = VirtualBaseBranch

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



Comment at: clang/lib/Sema/SemaChecking.cpp:5536
+// 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?]
 auto IsAllowedValueType = [&](QualType ValType) {

aaron.ballman wrote:
> 
Perhaps "but we do to help catch trivial type errors."



Comment at: clang/lib/Sema/SemaChecking.cpp:5578
+// the GNU atomics specification, but we enforce it, because if we didn't 
it
+// would be very confusing [For whom? How so?]
 Diag(ExprRange.getBegin(), diag::err_atomic_op_needs_trivial_copy)

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



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:
> ZarkoCA wrote:
> > Quuxplusone wrote:
> > > 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"?
> > 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? 
> 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.
@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.



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)

Quuxplusone wrote:
> Quuxplusone wrote:
> > This comment seems incorrectly translated.
> Perhaps just `// where they may be used by two-phase lookup.` (But this is 
> now just a nit.)
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.



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

Quuxplusone wrote:
> 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 

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

[Anshil Gandhi via Phabricator via cfe-commits]

gandhi21299 added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:5536
+// 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) {

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


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

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

[Zarko Todorovski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd8e5a0c42bd8: [clang][NFC] Inclusive terms: replace some 
uses of sanity in clang (authored by ZarkoCA).

Changed prior to commit:
  https://reviews.llvm.org/D114025?vs=388550=388587#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/include/clang/Sema/Lookup.h
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -126,7 +126,7 @@
   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) {
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -249,7 +249,7 @@
 }
 
 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();
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -326,8 +326,8 @@
 }
 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::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1670,9 +1670,10 @@
   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
Index: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -182,8 +182,7 @@
   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);
@@ -366,7 +365,7 @@
  const unsigned numArgs,
  const unsigned sizeArg,
  const char *fn) const {

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA marked an inline comment as done.
ZarkoCA added a comment.

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


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

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



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;

ZarkoCA wrote:
> Quuxplusone wrote:
> > 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"?
> 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? 
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.



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)

Quuxplusone wrote:
> This comment seems incorrectly translated.
Perhaps just `// where they may be used by two-phase lookup.` (But this is now 
just a nit.)


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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA added inline comments.



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:
> > 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"?
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? 


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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA updated this revision to Diff 388550.
ZarkoCA marked 3 inline comments as done.
ZarkoCA added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/include/clang/Sema/Lookup.h
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -126,7 +126,7 @@
   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) {
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -249,7 +249,7 @@
 }
 
 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();
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -326,8 +326,8 @@
 }
 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::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1670,9 +1670,10 @@
   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
Index: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -182,8 +182,7 @@
   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);
@@ -366,7 +365,7 @@
  const unsigned numArgs,
  const unsigned sizeArg,
  const char *fn) const {
-  // Sanity check for the correct number of 

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA updated this revision to Diff 388494.
ZarkoCA added a comment.

- Add FIXME to comments and fix grammar on one comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/include/clang/Sema/Lookup.h
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -126,7 +126,7 @@
   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) {
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -249,7 +249,7 @@
 }
 
 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();
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -326,8 +326,8 @@
 }
 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::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1670,9 +1670,10 @@
   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
Index: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -182,8 +182,7 @@
   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);
@@ -366,7 +365,7 @@
  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;

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA marked 3 inline comments as done.
ZarkoCA added a comment.

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.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

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




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

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



Comment at: clang/lib/Sema/SemaChecking.cpp:5536
+// 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?]
 auto IsAllowedValueType = [&](QualType ValType) {





Comment at: clang/lib/Sema/SemaChecking.cpp:5578
+// the GNU atomics specification, but we enforce it, because if we didn't 
it
+// would be very confusing [For whom? How so?]
 Diag(ExprRange.getBegin(), diag::err_atomic_op_needs_trivial_copy)




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

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

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

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.


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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA marked 13 inline comments as done.
ZarkoCA added a comment.

@Quuxplusone Thanks for thorough review.


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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA updated this revision to Diff 388332.
ZarkoCA added a comment.

- Address review comments
- Also rename some variables


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/include/clang/Sema/Lookup.h
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -126,7 +126,7 @@
   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) {
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -249,7 +249,7 @@
 }
 
 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();
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -326,8 +326,8 @@
 }
 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::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1670,9 +1670,10 @@
   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
Index: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -182,8 +182,7 @@
   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);
@@ -366,7 +365,7 @@
  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;

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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
 
-  // 

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA added a comment.

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.


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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA updated this revision to Diff 388207.
ZarkoCA edited the summary of this revision.
ZarkoCA added a comment.

- Reword comments based on suggestions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/include/clang/Sema/Lookup.h
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -126,7 +126,7 @@
   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) {
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -249,7 +249,7 @@
 }
 
 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();
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -326,8 +326,8 @@
 }
 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::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1670,7 +1670,7 @@
   if (isUnderconstrained(PrevN)) {
 IsSatisfied = true;
 
-// 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
 // missed the transition point.
 assert(!isUnderconstrained(N));
Index: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -182,8 +182,7 @@
   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, check for that here.
 return;
   } else if (CE->getNumArgs() == MaxArgCount) {
 const Expr *Arg = CE->getArg(CreateModeArgIndex);
@@ -366,7 +365,7 @@
  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;
 
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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.


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

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA added a comment.

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.


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

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

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?


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you!


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

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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA created this revision.
ZarkoCA added reviewers: aaron.ballman, dcoughlin, quinnp.
ZarkoCA added a project: clang.
Herald added subscribers: dexonsmith, martong.
ZarkoCA requested review of this revision.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114025

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/include/clang/Sema/Lookup.h
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/Store.cpp
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -126,7 +126,7 @@
   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) {
Index: clang/lib/StaticAnalyzer/Core/Store.cpp
===
--- clang/lib/StaticAnalyzer/Core/Store.cpp
+++ clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -249,7 +249,7 @@
 }
 
 SVal StoreManager::evalDerivedToBase(SVal Derived, const CastExpr *Cast) {
-  // Sanity check to avoid doing the wrong thing in the face of
+  // Verify that we avoid doing the wrong thing in the face of
   // reinterpret_cast.
   if (!regionMatchesCXXRecordType(Derived, Cast->getSubExpr()->getType()))
 return UnknownVal();
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -326,8 +326,8 @@
 }
 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::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1670,7 +1670,7 @@
   if (isUnderconstrained(PrevN)) {
 IsSatisfied = true;
 
-// 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
 // missed the transition point.
 assert(!isUnderconstrained(N));
Index: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -182,8 +182,7 @@
   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, check for that here.
 return;
   } else if (CE->getNumArgs() == MaxArgCount) {
 const Expr *Arg = CE->getArg(CreateModeArgIndex);
@@ -366,7 +365,7 @@
  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;
 
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
---