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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114025