This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Add 'Invalid' opcode and use it for throw/asm stmts
ClosedPublic

Authored by tbaeder on May 11 2023, 7:03 AM.

Details

Summary

As discussed the other day. This fixes the diagnostics for the test case in records.cpp.

Diff Detail

Event Timeline

tbaeder created this revision.May 11 2023, 7:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 7:03 AM
tbaeder requested review of this revision.May 11 2023, 7:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 7:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I've unfortunately not caught up enough to comment on the approach, but 'inline assembly' has the same restriction, so you might find MsAsmStmt and GCCAsmStmt to be equally as easy 'wins'.

"Unsupported" is a bit of a loaded term -- it could mean "this operation is not supported, YET" or it could mean "this operation is not and will not be supported, EVER". Perhaps something more like "InvalidInConstantExpr" would be more descriptive?

clang/test/AST/Interp/unsupported.cpp
3–5

Can get rid of some whitespace.

"Unsupported" is a bit of a loaded term -- it could mean "this operation is not supported, YET" or it could mean "this operation is not and will not be supported, EVER". Perhaps something more like "InvalidInConstantExpr" would be more descriptive?

I guess it would be more descriptive, but it could still mean that it is "not yet valid in a constant expression", so I guess I don't see the upside of using a longer opcode name.

tbaeder updated this revision to Diff 521346.May 11 2023, 9:29 AM

"Unsupported" is a bit of a loaded term -- it could mean "this operation is not supported, YET" or it could mean "this operation is not and will not be supported, EVER". Perhaps something more like "InvalidInConstantExpr" would be more descriptive?

I guess it would be more descriptive, but it could still mean that it is "not yet valid in a constant expression", so I guess I don't see the upside of using a longer opcode name.

I don't feel strongly; it's easy enough to rename if we think it's causing confusion. FWIW, my first thought was "Oh, we're planning to support throw expressions in constant expressions? Please don't tell WG21." I'm used to seeing "invalid" for things that are never valid and "unsupported" for things that aren't supported but might be someday. However, I also see we use "unsupported" in the same sense you're using it here in some of our diagnostics, so I'm fine with whatever you want to go with.

clang/test/AST/Interp/records.cpp
341

Oh interesting -- does the old constexpr interpreter think the destructor is called at the end of the block as opposed to at the end of the full expression with the temporary?

"Unsupported" is a bit of a loaded term -- it could mean "this operation is not supported, YET" or it could mean "this operation is not and will not be supported, EVER". Perhaps something more like "InvalidInConstantExpr" would be more descriptive?

I guess it would be more descriptive, but it could still mean that it is "not yet valid in a constant expression", so I guess I don't see the upside of using a longer opcode name.

I don't feel strongly; it's easy enough to rename if we think it's causing confusion. FWIW, my first thought was "Oh, we're planning to support throw expressions in constant expressions? Please don't tell WG21." I'm used to seeing "invalid" for things that are never valid and "unsupported" for things that aren't supported but might be someday. However, I also see we use "unsupported" in the same sense you're using it here in some of our diagnostics, so I'm fine with whatever you want to go with.

I didn't know about that distinction, renaming to invalid is fine with me.

tbaeder updated this revision to Diff 521568.May 11 2023, 11:28 PM
tbaeder marked an inline comment as done.
tbaeder retitled this revision from [clang][Interp] Add 'Unsupported' opcode and use it for throw stmts to [clang][Interp] Add 'Invalid' opcode and use it for throw/asm stmts.
tbaeder added inline comments.May 11 2023, 11:32 PM
clang/test/AST/Interp/records.cpp
341

I think it's the other way around and the new one does that (expected is the new one). Need to investigate that.

aaron.ballman accepted this revision.May 12 2023, 7:21 AM

LGTM

clang/test/AST/Interp/records.cpp
341

Ah oops, you're right, I did have that backwards. That makes more sense, thanks. :-)

This revision is now accepted and ready to land.May 12 2023, 7:21 AM
This revision was landed with ongoing or failed builds.Jul 26 2023, 12:00 AM
This revision was automatically updated to reflect the committed changes.