This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Start implementing record types
ClosedPublic

Authored by tbaeder on Sep 16 2022, 10:27 AM.

Details

Summary

This is just a humble beginning of course.

Posting this now for your weekend reading. And also to keep the individual patches small.

Diff Detail

Event Timeline

tbaeder created this revision.Sep 16 2022, 10:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 10:27 AM
tbaeder requested review of this revision.Sep 16 2022, 10:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 10:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
erichkeane added inline comments.Sep 16 2022, 10:39 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
316

I THINK Member is a ValueDecl because it could be a member function, right? So forcing it to be a FieldDecl here is likely not valid. Perhaps in the 'non-FieldDecl' case we could jsut return false for now?

ALSO, don't do a dyn_cast followed by an assert, cast will do the assert for you.

659

Eh?

tbaeder added inline comments.Sep 16 2022, 12:30 PM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
316

Right, I was just trying to limit the code to the subset I've implemented for now. I can try to make it more defensive.

659

As far as I understand, actually visiting the body of the constructor will require a bit more setup work of this pointers and such. So for now, not implemented and a FIXME.

tbaeder updated this revision to Diff 460972.Sep 16 2022, 11:36 PM
tbaeder updated this revision to Diff 460974.Sep 16 2022, 11:58 PM
tbaeder updated this revision to Diff 460975.Sep 17 2022, 12:08 AM
tbaeder updated this revision to Diff 460998.Sep 17 2022, 6:04 AM
tbaeder updated this revision to Diff 461001.Sep 17 2022, 6:09 AM
shafik added inline comments.Sep 17 2022, 3:29 PM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
316

Also I believe this can also be an IndirectFieldDecl (anonymous union members) or a ValueDecl for static data members.

Maybe outline what is left to fill in a comment?

672

This section of code looks duplicated w/ the above, can it be factored out or will they diverge as you fill in more details?

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

How about also have a field that is a struct and initializing that.

Also using initializer lists in in class member initializers and also designated initializers as well.

I am not sure if unions works yet but anon union members as well.

tbaeder updated this revision to Diff 461148.Sep 19 2022, 12:02 AM
tbaeder marked 2 inline comments as done.
tbaeder added inline comments.Sep 19 2022, 12:20 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
672

The version above will go away in my next patch to record implementation.

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

Struct initializers here aren't supported yet (that's what the "Handle initializer for non-primitive values" assert in VisitRecordInitializer is about). I added a FIXME comment and a test for designated initializers.

aaron.ballman added inline comments.Sep 21 2022, 9:03 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
316

There are quite a few kinds of declarations, but many of them don't matter or only matter in special circumstances. Like StaticAssertDecl shouldn't be something we can visit here, but technically TypedefDecl can because of VLA evaluations. MSPropertyDecl is one that could possibly matter as well.

640

Do we have to do anything special if the ctor is an inheriting ctor?

685

CXXMemberCallExpr as well?

687

What if this comes back as nullptr (does getFunction() handle that gracefully)?

tbaeder marked an inline comment as done.Sep 21 2022, 10:39 AM
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
640

I have a feeling we do, but I'll tackle inheritance and virtual functions later.

685

Yup, that's unhandled here right now it seems, I can add it to this patch or add it in a later one.

687

It does not :) I was relying on a crash somewhere when I have a test case for it.

aaron.ballman accepted this revision.Sep 26 2022, 2:21 PM

LGTM aside from a minor nit, we can handle the other cases in follow-ups.

clang/lib/AST/Interp/ByteCodeExprGen.cpp
640

Okay, that seems reasonable to me.

685

I'm fine doing it in a follow-up.

687

I'd rather use an explicit assert for that instead of relying on a crash; it's easier to spot the assert in that case.

This revision is now accepted and ready to land.Sep 26 2022, 2:21 PM
tbaeder marked 2 inline comments as done.Sep 26 2022, 9:40 PM
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
687

I forgot I hadn't posted this to phab yet, but https://reviews.llvm.org/D134699 introduces a getFunction that asserts that the given FunctionDecl is not null.

aaron.ballman added inline comments.Sep 28 2022, 5:45 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
687

Ah, okay, that works fine for me then.

This revision was landed with ongoing or failed builds.Oct 14 2022, 1:22 AM
This revision was automatically updated to reflect the committed changes.
tbaeder marked an inline comment as done.