Skip to content
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

serialization: support passing extra args to fields in DSL #9287

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jeffro256
Copy link
Contributor

This PR is upstreaming changes in the Seraphis lib here: UkoeHB#39. The changes to the serialization header allow clean passing of extra arguments to field serialization in the DSL. This is used mainly to pass implied sizes of containers during deserialization to make the format more compact. For example, if my object has two containers A & B which must be the same size, I can serialize only the size of container A. Then, during deserialization, when I deserialize A, I can then use A's size to deserialize B.

Depends on #9286.

This PR is upstreaming changes in the Seraphis lib here: UkoeHB#39. This header adds a macro `VA_ARGS_COMMAPREFIX`
which, when passed `__VA_ARGS__`, expands to `, __VA_ARGS__` unless the length of `__VA_ARGS__` is 0, in which case it expands to nothing. This
macro is useful for passing/declaring optional function arguments.
This PR is upstreaming changes in the Seraphis lib here: UkoeHB#39. The changes to the serialization header allow clean passing
of extra arguments to field serialization in the DSL. This is used mainly to pass implied sizes of containers during deserialization to make the format more
compact. For example, if my object has two containers A & B which must be the same size, I can serialize only the size of container A. Then, during
deserialization, when I deserialize A, I can then use A's size to deserialize B.

Depends on monero-project#9286.
Copy link
Contributor

@vtnerd vtnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this additional code isn't required to achieve the array logic - ringct already does this. I suppose this makes it slightly easier to group the arrays though.

// front if more than one argument, else nothing.
// If __VA_OPT__ supported, use that. Else, use GCC's ,## hack
#if VA_OPT_SUPPORTED
# define VA_ARGS_COMMAPREFIX(...) __VA_OPT__(,) __VA_ARGS__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why even have this extra mode if the compatibility mode forces a 1+ argument anyway? Basically, you are limited by what the compatibility mode can do anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compatibility mode shouldn't force a 1+ argument. __VA_OPT__(x) should expand to nothing if no variadic arguments are provided: https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but:

VA_ARGS_COMMAPREFIX(...) , ## __VA_ARGS__

should have a dangling comma ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On GCC-style compilers , ## __VA_ARGS__ also expands to nothing if there are no variadic args.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that's what the comment says.

} \
template <bool W, template <bool> class Archive> \
bool do_serialize_object(Archive<W> &ar, stype &v) { \
#define BEGIN_SERIALIZE_OBJECT_FN(stype, ...) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the first argument, you have to list the typename and variable name? A little funky, but it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!

@jeffro256 jeffro256 closed this Apr 23, 2024
@jeffro256 jeffro256 reopened this Apr 23, 2024
@jeffro256
Copy link
Contributor Author

Oops closed wrong one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants