hokein added a comment.
In D141580#4052496 <https://reviews.llvm.org/D141580#4052496>, @sammccall wrote:
> 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.
`Sema::Act*` have a mixed usage of returning {*Result`, `void`, a pointer,
`bool`}, unclear the common practice there. Changing `ActOnTemplatedFriendTag`
and `ActOnTag` return-type seems be more aligned with the other `Sema::Act*`
methods used in the `ParseClassSpecifier` context. And these two APIs are not
widely used, it is probably fine.
> (Aaron, this is your call, just wanted to make sure you're aware of the cost)
To me, I don't have a strong preference, I'm fine with either solution.
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2069-2078
+ if (auto *TagDecl = 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,
----------------
aaron.ballman wrote:
> hokein wrote:
> > hokein wrote:
> > > aaron.ballman wrote:
> > > > sammccall wrote:
> > > > > 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.
> > > > I think this is the wrong approach to fixing the issue. None of the
> > > > other assignments to `TagOrTempResult` test for nullptr first, such as:
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2037
> > > > and
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2029.
> > > >
> > > > An `ActionResult` can be both valid/invalid *and* usable/unusable (and
> > > > a `DeclResult` is a kind of `ActionResult`). When it's assigned *any
> > > > pointer value* (including nullptr), it's a valid `ActionResult` but
> > > > it's not a usable `ActionResult`
> > > > (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/Ownership.h#L166).
> > > >
> > > > I think the correct fix is to find the places assuming a valid
> > > > `ActionResult` means a nonnull pointer from `get()`, such as
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2121
> > > > and switch them over to looking for a usable `ActionResult` instead.
> > > Thanks for the comment!
> > >
> > > > None of the other assignments to TagOrTempResult test for nullptr first
> > >
> > > I think the problem here is:
> > > - most of the `ActOn*` methods return a `DeclResult` (and inside these
> > > method implementations, they return an Invalid `DeclResult` if the Decl
> > > is nullptr), so `TagOrTempResult = Actions.ActOn*` is a fine pattern.
> > > - Only two exceptions `ActOnTagDecl` and `ActOnTemplatedFriendTag`, they
> > > return a `Decl*`, so the `TagOrTempResult = Action.ActOn*` is a very
> > > suspicious pattern. (IMO, they're not intentional, likely bugs).
> > >
> > > > such as:
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2037
> > > > and
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2029.
> > >
> > > The first one is the other exception; the second one is not
> > > (ActOnExplicitInstantiation returns a `DeclResult`).
> > >
> > > > I think the correct fix is to find the places assuming a valid
> > > > ActionResult means a nonnull pointer from get(), such as
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2121
> > > > and switch them over to looking for a usable ActionResult instead.
> > >
> > > This is one of the solutions. But instead of justifying every places of
> > > DeclResult, I think we should fix the two exception places (by adding the
> > > nullptr check in
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2037
> > > and
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDeclCXX.cpp#L2069),
> > > what do you think?
> > I have updated the patch to check nullptr for ActOnTemplatedFriendTag as
> > well. Let me know what you think (I'm also happy to do the change).
> Actually, I think the best solution is to make those APIs return a correctly
> constructed (invalid) `DeclResult` rather than a `Decl *` that turns into a
> valid-but-unusable `DeclResult`. I tried the change out locally and it seems
> to have the same fallout as your changes with only a small amount of extra
> work involved.
>
> I have this done locally, would you like me to go ahead and clean it up to
> commit it or hand a patch off to you to finish? Or do you think that's not a
> good approach?
> Actually, I think the best solution is to make those APIs return a correctly
> constructed (invalid) DeclResult rather than a Decl * that turns into a
> valid-but-unusable DeclResult. I tried the change out locally and it seems to
> have the same fallout as your changes with only a small amount of extra work
> involved.
Yeah, I considered this approach, but I was not convinced myself is
significantly better than the current approach, and it requires some more
changes.
> I have this done locally, would you like me to go ahead and clean it up to
> commit it or hand a patch off to you to finish? Or do you think that's not a
> good approach?
Thanks, since you already have a patch, please go ahead (if you think it is
better). I'm happy to review.
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
