aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LG pending test coverage being ready to land
================
Comment at: clang/test/AST/Interp/records.cpp:317-318
{
- auto T = Test(Arr, Pos);
+ Test(Arr, Pos);
// End of scope, should destroy Test.
}
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > tbaeder wrote:
> > > aaron.ballman wrote:
> > > > tbaeder wrote:
> > > > > aaron.ballman wrote:
> > > > > > Nit: nothing actually tests that this object is destroyed
> > > > > > correctly. Here's an interesting test to consider:
> > > > > > ```
> > > > > > struct S {
> > > > > > constexpr S() {}
> > > > > > constexpr ~S() noexcept(false) { throw 12; }
> > > > > > };
> > > > > >
> > > > > > constexpr int f() {
> > > > > > S{};
> > > > > > return 12;
> > > > > > }
> > > > > >
> > > > > > static_assert(f() == 12);
> > > > > > ```
> > > > > > That should fail because `~S()` would hit the `throw` expression
> > > > > > and thus is not valid. Note, you'll need to add
> > > > > > `-Wno-invalid-constexpr` to your test to avoid the
> > > > > > warning-defaults-to-error about the destructor never producing a
> > > > > > constant expression.
> > > > > There are multiple reasons why that sample is not rejected right now,
> > > > > one I can easily fix in a follow-up patch, the other one would
> > > > > actually require us to recognize the `throw` and reject it with a
> > > > > proper diagnostic.
> > > > We should definitely fix the `throw` at some point, but any of the
> > > > dynamically reachable problematic constructs would work (`dynamic_cast`
> > > > whose type would throw, invocation of the `va_arg` macro,
> > > > `reinterpret_cast`, etc)
> > > Yes, I think we need a new opcode for that so we only emit the diagnostic
> > > when such a construct is actually executed.
> > Oh yeah, you'll definitely need that, a whole pile of the constexpr rules
> > are based around code reachability.
> >
> > Are you saying you've got no way to test this until you implement that
> > opcode?
> With https://reviews.llvm.org/D150040 applied, it gets properly rejected,
> just the diagnostics are off. I can add the test and reorder the commits.
SGTM, thanks!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147591/new/
https://reviews.llvm.org/D147591
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
