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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
