This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Record initialization via conditional operator
ClosedPublic

Authored by tbaeder on Jan 11 2023, 7:01 AM.

Details

Summary

Factor this out into a visitConditional() that calls the given visitor function.

Diff Detail

Event Timeline

tbaeder created this revision.Jan 11 2023, 7:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2023, 7:01 AM
tbaeder requested review of this revision.Jan 11 2023, 7:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2023, 7:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
erichkeane added inline comments.Jan 11 2023, 7:07 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
920

This meant to be here?

921

Is this condition supposed to be just a 'visit', or should this be using the functor?

clang/lib/AST/Interp/ByteCodeExprGen.h
200

I'd probably rather make this a template taking a functor, rather than bringing in the atrocities that come with std::function.

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

Did this not work before? I don't see what part of teh code changes makes this work?

tbaeder added inline comments.Jan 11 2023, 7:16 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
920

Nope! :(

921

The visit is correct here, the functor is just for the true/false expr.

clang/lib/AST/Interp/ByteCodeExprGen.h
200

Alright, will look into that

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

We call visitInitializer also when the return value is constructed in the calling function, so the return statement in getS() exercises the code added.

tbaeder marked 2 inline comments as done.Jan 13 2023, 7:17 AM
tbaeder updated this revision to Diff 489720.Jan 17 2023, 12:36 AM
tbaeder added inline comments.Jan 17 2023, 12:37 AM
clang/lib/AST/Interp/ByteCodeExprGen.h
200

Done. Not sure if this is exactly how you meant it to look. Feels a bit inelegant.

erichkeane added inline comments.Jan 17 2023, 7:00 AM
clang/lib/AST/Interp/ByteCodeExprGen.h
200

This actually probably needs to be a non-member now, since its only used in the .cpp.

while I DO dislike that it was using std::function (probably because of captures in the lambdas?), I now suspect this is worse if we can't have it in the header, right?

tbaeder updated this revision to Diff 504079.Mar 10 2023, 3:15 AM
tbaeder marked an inline comment as done.Mar 19 2023, 12:24 AM

Ping

aaron.ballman added inline comments.Mar 20 2023, 12:07 PM
clang/lib/AST/Interp/ByteCodeExprGen.h
199–200

The template definition isn't available within the header file, so this is fragile (impossible to instantiate from anything but ByteCodeExprGen.cpp).

tbaeder added inline comments.Mar 24 2023, 1:21 AM
clang/lib/AST/Interp/ByteCodeExprGen.h
199–200

What's the alternative? If I make it a static function in ByteCodeExprGen.cpp, I'd have to make a lot of members of ByteCodeEmitter etc. public which is not a very clean solution. Moving the definition into the header file isn't very nice either, all the others are in the source file.

erichkeane added inline comments.Mar 24 2023, 6:58 AM
clang/lib/AST/Interp/ByteCodeExprGen.h
199–200

What about making it a static template in ByteCodeExprGen.cpp, but make it a 'friend' here? I think that would work, wouldn't it? Something like: https://godbolt.org/z/ofYbGvfYa

You unfortunately cannot make it static, but as it is a template, at least it is inline. WDYT?

tbaeder added inline comments.Mar 27 2023, 5:56 AM
clang/lib/AST/Interp/ByteCodeExprGen.h
199–200

I tried that but I keep running into linker problems:

mold: error: undefined symbol: lib/libclangAST.a(ByteCodeExprGen.cpp.o): bool clang::interp::visitConditional<clang::interp::ByteCodeExprGen<clang::interp::ByteCodeEmitter>::VisitAbstractConditionalOperator(clang::AbstractConditionalOperator const*)::{lambda(clang::Expr const*)#1}>(clang::interp::ByteCodeExprGen<clang::interp::ByteCodeEmitter>*, clang::AbstractConditionalOperator const*, clang::interp::ByteCodeExprGen<clang::interp::ByteCodeEmitter>::VisitAbstractConditionalOperator(clang::AbstractConditionalOperator const*)::{lambda(clang::Expr const*)#1})

(and another one for EvalEmitter). But I can't investigate more before Wednesday.

tbaeder updated this revision to Diff 510180.Mar 31 2023, 10:19 PM

Screw it, just use a llvm::function_ref.

aaron.ballman accepted this revision.Apr 3 2023, 5:48 AM

I'm fine with this approach.

This revision is now accepted and ready to land.Apr 3 2023, 5:48 AM