Repository navigation
[hls-fuzzer][NFC] Make OutputTransferFn and TransferFn more alike - #970
Conversation
`OutputTransferFn` previously was this odd class that was kind of similarly constructible as `TransferFn`, but in usage was more similar to `OpaqueTransferFn`. A side-effect of this was that the class was less type-safe than it could be and user-code occaisonally had to manually do casts from and to `void*`. This PR both improves the interface of `OpaqueTransferFn` to make it more type safe and then improves `OutputTransferFn` to be as similar to `TransferFn` as possible (making the API more consistent). A new `OpaqueOutputTransferFn` now also mirrors `OpaqueTransferFn`
d4aece6 to
ca685a2
Compare
| /// TypingContext(const ASTNode &, | ||
| /// const TypedContextTuple<ASTNode, TypingContext> &) | ||
| template <typename TypingContext, typename F> | ||
| OpaqueOutputTransferFn(llvm::identity<TypingContext>, F &&f) |
There was a problem hiding this comment.
what does llvm identity do here?
There was a problem hiding this comment.
As stated in the comment, its really just a dummy value such that users can specify a template argument for the constructor.
Effectively this works around by there not being a syntax to specify template arguments for a constructor in C++, meaning one has to use template type deduction to do so.
llvm::identity happens to be a lightweight type and value that is default constructible, meaning one can specify a type using llvm::identity<T>{}.
Its not ideal, but this constructor is unlikely to be used by users unless one creates composite type systems like the ConjunctionTypeSystem
| const SubElementsTuple &nonTerminals, | ||
| const ContextTuple &tuple)) | ||
| : dep(std::move(dep)), computationFn(computationFn), | ||
| F &&f) |
There was a problem hiding this comment.
F is too uninformative maybe use ConcreteTransferFn?
| for (auto &&[unwrappedEl, wrappedEl] : | ||
| llvm::zip_equal(unwrapped, tuple)) { | ||
| if (!wrappedEl) { | ||
| unwrappedEl = nullptr; | ||
| continue; | ||
| } | ||
| unwrappedEl = reinterpret_cast<const TypingContext *>(wrappedEl); | ||
| } |
There was a problem hiding this comment.
unwrapped is an empty iterable, no? I didn't get what is being iterated
There was a problem hiding this comment.
Its actually an std::array<const TypingContext*, ...>. All that is happening here is that we are filling unwrappedEl by casting the const void* from tuple to const TypingContext*.
I've simplified the code a bit
OutputTransferFnpreviously was this odd class that was kind of similarly constructible asTransferFn, but in usage was more similar toOpaqueTransferFn. A side-effect of this was that the class was less type-safe than it could be and user-code occaisonally had to manually do casts from and tovoid*.This PR both improves the interface of
OpaqueTransferFnto make it more type safe and then improvesOutputTransferFnto be as similar toTransferFnas possible (making the API more consistent). A newOpaqueOutputTransferFnnow also mirrorsOpaqueTransferFn