[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