Skip to content

Conversation

@dexX7
Copy link

@dexX7 dexX7 commented Oct 17, 2014

All tests pass and building, using the client works as expected.

The Format__MP tests should cover almost everything, tests for strToInt64 were copied from zathras, rounduint64 has none.

strprintf was an alias for tfm::format in util.h which refers to tinyformat.h which seems to have many boost dependencies.

I'm not sure about function names. Upper case, lower case, ...?

Also note: FormatDivisibleMP and FormatIndivisibleMP were in namespace mastercore. If they should be in an explicit mastercore namespace, then I suggest to remove the MP suffix.

@m21
Copy link

m21 commented Oct 17, 2014

Nice to have the tests as part of make fore sure.
I am not crazy about the "transformers" name. Maybe mastercore_format.cpp or similar?
I kinda prefer shorter filenames to long ones, but that's a very weak comment on my part.
Just FYI : I see that at Bitcoin master they are splitting util.cpp into: util.cpp utilmoneystr.cpp utilstrencodings.cpp utiltime.cpp

and need @zathras-crypto and @faizkhan00 on this as well. Thanks!

@dexX7
Copy link
Author

dexX7 commented Oct 18, 2014

Agreed, mastercore_format is a much better choice. It may also be considered to move into a seperate directory and get rid of the mastercore_ prefix. By using one or more namespaces, hints such as __MP() could be further removed.

@dexX7 dexX7 force-pushed the mcore-formatters-and-tests branch from 00446c4 to ce1913c Compare October 20, 2014 04:48
@dexX7 dexX7 changed the title Move formatters etc. into own file + tests WIP: Move formatters etc. into own file + tests Oct 21, 2014
@dexX7
Copy link
Author

dexX7 commented Oct 23, 2014

Replaced by #180, #182 and #183.

@dexX7 dexX7 closed this Oct 23, 2014
@dexX7 dexX7 deleted the mcore-formatters-and-tests branch November 7, 2014 17:32
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.

2 participants