Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D141497 Diff 511301 clang/lib/AST/Interp/ByteCodeExprGen.h

Changeset View

Standalone View

View Options

clang/lib/AST/Interp/ByteCodeExprGen.h

Show First 20 Lines • Show All 175 Lines • ▼ Show 20 Lines if (!this->emitThis(I))
return false; return false;
if (!visitInitializer(I)) if (!visitInitializer(I))
return false; return false;
return this->emitPopPtr(I); return this->emitPopPtr(I);
} }
bool visitConditional(const AbstractConditionalOperator *E,
llvm::function_ref<bool(const Expr *)> V);
erichkeaneUnsubmitted
Not Done
ReplyInline Actions

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

erichkeane: I'd probably rather make this a template taking a functor, rather than bringing in the…
tbaederAuthorUnsubmitted
Done
ReplyInline Actions

Alright, will look into that

tbaeder: Alright, will look into that
tbaederAuthorUnsubmitted
Done
ReplyInline Actions

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

tbaeder: Done. Not sure if this is exactly how you meant it to look. Feels a bit inelegant.
erichkeaneUnsubmitted
Not Done
ReplyInline Actions

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?

erichkeane: This actually probably needs to be a non-member now, since its only used in the .cpp. while I…
aaron.ballmanUnsubmitted
Not Done
ReplyInline Actions

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

aaron.ballman: The template definition isn't available within the header file, so this is fragile (impossible…
tbaederAuthorUnsubmitted
Done
ReplyInline Actions

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.

tbaeder: What's the alternative? If I make it a static function in `ByteCodeExprGen.cpp`, I'd have to…
erichkeaneUnsubmitted
Not Done
ReplyInline Actions

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?

erichkeane: What about making it a static template in `ByteCodeExprGen.cpp`, but make it a 'friend' here?
tbaederAuthorUnsubmitted
Done
ReplyInline Actions

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: I tried that but I keep running into linker problems: ``` mold: error: undefined symbol…
/// Creates a local primitive value. /// Creates a local primitive value.
unsigned allocateLocalPrimitive(DeclTy &&Decl, PrimType Ty, bool IsMutable, unsigned allocateLocalPrimitive(DeclTy &&Decl, PrimType Ty, bool IsMutable,
bool IsExtended = false); bool IsExtended = false);
/// Allocates a space storing a local given its type. /// Allocates a space storing a local given its type.
std::optional<unsigned> allocateLocal(DeclTy &&Decl, bool IsExtended = false); std::optional<unsigned> allocateLocal(DeclTy &&Decl, bool IsExtended = false);
private: private:
▲ Show 20 Lines • Show All 246 Lines • Show Last 20 Lines