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
