sammccall added a comment.
To offer the opposing argument: if DeclResult is just a bad idea, then using it
consistently/right might be worse than using it as little as possible.
FWIW, I find every piece of code that produces/consumes the `*Result` to be
extremely confusing (the semantics of the two different sentinel values are
never obvious) and use of the type in more APIs means more callers have to deal
with this tri-state logic and bugs like this one.
So spreading DeclResult around where it's not strictly needed feels like the
wrong direction to be, and i find the original version of the patch easier to
understand. It may even be possible to switch entirely to pointers here.
(Aaron, this is your call, just wanted to make sure you're aware of the cost)
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2069
// Declaration or definition of a class type
- TagOrTempResult = Actions.ActOnTag(
- getCurScope(), TagType, TUK, StartLoc, SS, Name, NameLoc, attrs, AS,
- DS.getModulePrivateSpecLoc(), TParams, Owned, IsDependent,
- SourceLocation(), false, clang::TypeResult(),
- DSC == DeclSpecContext::DSC_type_specifier,
- DSC == DeclSpecContext::DSC_template_param ||
- DSC == DeclSpecContext::DSC_template_type_arg,
- &SkipBody);
+ if (auto *TagDecl = Actions.ActOnTag(
+ getCurScope(), TagType, TUK, StartLoc, SS, Name, NameLoc, attrs,
AS,
----------------
just so I understand:
initial state of TagOrTempResult is (null, invalid)
if ActOnTag returns ptr, we set it to (ptr, valid)
if ActOnTag returns null, then the old code would set it to (null, valid) and
the new code would leave it as (null, invalid)
ActOnTag returns null in error cases (diagnostic has been emitted).
We check validity below when deciding whether to use the returned pointer (so
we're passing null around at that point).
I can't really infer what the invariants here were originally meant to be, and
obviously we're not crashing all over the place, and the diagnostics look like
a wash to me. But the new logic seems much more sane.
================
Comment at: clang/test/SemaCXX/rdar42746401.cpp:7
-::b<0> struct c::d // expected-error{{incomplete type}} expected-error{{cannot
combine}} expected-error{{expected unqualified-id}}
----------------
hokein wrote:
> The removed diagnostic is "cannot combine with previous 'type-name'
> declaration specifier" which is bogus.
That doesn't sound bogus to me: `::b<0>` is a type name, `struct c::d` is a
type name, and you can't combine two type-names in a decl-specifier-sequence.
On the other hand it's a follow-on diagnostic in a situation where it's
probably not useful.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141580/new/
https://reviews.llvm.org/D141580
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
