-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use full block type name in registry and serialization #508
Conversation
ce52d9f
to
2804664
Compare
return _blockTypeHandlers[blockType]; | ||
} | ||
std::vector<std::string> _blockTypes; | ||
std::map<std::string, TBlockTypeHandler, std::less<>> _blockTypeHandlers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically a std::map<string, pair<string, func>>
. Why do we need both strings (key and it's alias)?
If needed, maybe add a small comment on the layout and few examples that developers should expect to find in this map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When writing out the block type (GRC/message), the meta::type_name on the block returns the long name (e.g. SomeBlockImpl<T, true, 42>
>) and BlockRegistry::blockTypeName("SomeBlockImpl<T, true, 42>")
does a lookup in this map and returns the alias if set (e.g. SomeBlock<T>
). This could also be a separate map<string, string>
, given that types with alias are the exception.
2804664
to
2ba5e00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frankosterfeld nicely done. I like the more stream-lined API and auto-naming.
There are some comments, adjustments request outlined above, I'd ask you to address but these are minor and should take to much time.
Maybe @ivan-cukic could also cross-check the semantic and API before merging this because this affects his issue as well: fair-acc/opendigitizer#256
free(demangled_name); | ||
return typeid(T).name(); | ||
} | ||
return detail::makePortableTypeName(detail::local_type_name<T>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not inlining the detail::makePortableTypeName()
functionality right here since it's not used elsewhere.
2ba5e00
to
9a2183e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frankosterfeld thanks for the changes! Much appreciated!
f45489c
to
fbee1d0
Compare
In the BlockRegistry, store the registered blocks in a flat structure, treating different specializations of the same block template (e.g., "Block<double>" and "Block<float>") as separate types. In GRC and messages, also use the full type ("Block<double>"), instead of keeping template name ("Block") and parameters ("double") separate. Signed-off-by: Frank Osterfeld <[email protected]>
fbee1d0
to
c323e09
Compare
|
In the BlockRegistry, store the registered blocks in a flat structure, treating different specializations of the same block template (e.g., "Block" and "Block") as separate types.
In GRC and messages, also use the full type ("Block"), instead of keeping template name ("Block") and parameters ("double") separate.