Skip to content

Conversation

czurnieden
Copy link
Contributor

Had been requested a long time ago (#2 ).

@czurnieden czurnieden requested a review from nijtmans April 6, 2019 17:14
@@ -624,9 +636,10 @@
#endif

#if defined(BN_MP_MUL_C)
# define BN_FAST_S_MP_MUL_DIGS_C
Copy link
Member

Choose a reason for hiding this comment

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

lulz looks like that comment mentioning "fast_s_mp_mul_digs" triggers dep.pl

Copy link
Contributor Author

@czurnieden czurnieden Apr 7, 2019

Choose a reason for hiding this comment

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

Oh, great *sigh*
Had merge conflicts with tommath_class.hand callgraph.txt and thought the simplest way would be to just delete their content and let make new_file do its thing.
Which seemed to work.
What to do: change my comment or patch dep.pl to ignore comments (which is quite complicated if you want to get all edgecases)?
Oh, you already merged it.
I'm too slow ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Which seemed to work.

it worked indeed.

What to do: change my comment or patch dep.pl to ignore comments

very good question!
@karel-m you're the perl guy I know, is there an easy way to fix this in dep.pl?

@sjaeckel sjaeckel merged commit 2033fb9 into libtom:develop Apr 7, 2019
@@ -335,6 +335,7 @@ int mp_sub(const mp_int *a, const mp_int *b, mp_int *c);

/* c = a * b */
int mp_mul(const mp_int *a, const mp_int *b, mp_int *c);
int mp_balance_mul(const mp_int *a, const mp_int *b, mp_int *c);
Copy link
Member

Choose a reason for hiding this comment

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

@czurnieden Is it intended that mp_balance_mul is exposed in the public api or should this rather go to tommath_private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it intended that mp_balance_mul is exposed in the public api

But everything is exposed?!
I know you fight it tooth and nails but LTM should make it explicit by using the methods offered by modern C-compilers and not by "hiding" it in another header.
But that discussion is for another day and another version.

should this rather go to tommath_private

You really don't need to tiptoe around me if you want something done, I'm a friendly bear ;-)

But as this PR is already merged and it is only a small, non-essential problem I would like to put it piggyback (but with a distinct commit for blaming) on one of the other PRs.

Copy link
Member

Choose a reason for hiding this comment

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

@czurnieden I really care about the exposed API to be honest. This is what keeps the library nice and usable. To make this more clear private functions should be hidden symbols or at least be prefixed by s_.

I have some other minor comments incoming in a small PR. But no big deal.

@czurnieden czurnieden deleted the bn_mul_balance branch May 8, 2019 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants