[PATCH] D141580: [clang] Don't consider a nullptr returned from ActOnTag() as a valid result in ParseClassSpecifier.

[Sam McCall via Phabricator via cfe-commits]

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