Added support for binary modifier

[gcflymoto]

Split pull request for bin modifier support

[vitaut]

Great, thanks!

I have a few comments:

1. In the table describing integer types gcflymoto@e4ffc06#diff-0ced3dfd44ba156272e6db34a5247462R238, the descriptions for 'b' and 'B' are swapped, e.g. the description for 'b' should say "... lower-case 0b ..." rather than " ... upper-case 0B ...".
2. Could you split this pull request into several, one for each new feature, e.g. one PR that adds the binary modifier, another that adds support for older GCC, etc? This way it would be easier to track and review them. I think the binary modifier part looks great, but have some questions regarding the rest.

Thanks again,
Victor

[gcflymoto]

Sure. I'll separate them

Greg

On Nov 8, 2013, at 5:44 PM, vitaut notifications@github.com wrote:

Great, thanks!

I have a few comments:

In the table describing integer types gcflymoto/format@e4ffc06#diff-0ced3dfd44ba156272e6db34a5247462R238, the descriptions for 'b' and 'B' are swapped, e.g. the description for 'b' should say "... lower-case 0b ..." rather than " ... upper-case 0B ...".

Could you split this pull request into several, one for each new feature, e.g. one PR that adds the binary modifier, another that adds support for older GCC, etc? This way it would be easier to track and review them. I think the binary modifier part looks great, but have some questions regarding the rest.

Thanks again,
Victor

—
Reply to this email directly or view it on GitHub.

[vitaut]

Merged in with some modifications.

In particular I removed 'B' specifier and binu because these have no effect on output when there is no prefix. It is different from hex ('x' & 'X') where letters 'a' - 'f' are used to denote digits and they can be either uppercase or lowercase. Both 'x' & 'X' use the same prefix "0x" so I think having a single prefix for binary ("0b") as well is more consistent. Anyway one can always specify uppercase (or any other) prefix explicitly, e.g. "0B{:b}".

I've also removed tests for unsigned long long for now and added more tests for binary formatting. Feel free to submit unsigned long long support (and any other changes) in different pull requests.

Thanks,
Victor

[gcflymoto]

Darn, this is not what I was going for. I was going for being as close to C++ streams as possible, where one can do

std::cout << std::showbase << std::uppercase << std::hex;Which should have an equivalent as
fmt::Format("{:#X}")The point is not there is a workaround with "0X{:X}", the point is to have the mechanisms equivalent to streams. I guess Ill have to continue maintaining my fork which will have "{:#X}" and "{:#B}" do the right capitalization.

On Thursday, November 14, 2013 8:59 AM, vitaut notifications@github.com wrote:

Merged in with some modifications.
In particular I removed 'B' specifier and binu because these have no effect on output when there is no prefix. It is different from hex ('x' & 'X') where letters 'a' - 'f' are used to denote digits and they can be either uppercase or lowercase. Both 'x' & 'X' use the same prefix "0x" so I think having a single prefix for binary ("0b") as well is more consistent. Anyway one can always specify uppercase (or any other) prefix explicitly, e.g. "0B{:b}".
I've also removed tests for unsigned long long for now and added more tests for binary formatting. Feel free to submit unsigned long long support (and any other changes) in different pull requests.
Thanks,
Victor
—
Reply to this email directly or view it on GitHub.

[vitaut]

I don't have anything against having 'B' specifier, but I think this needs to be consistent with 'X'. Currently 'X' uses "0x" prefix and that's one of the reasons I removed 'B' for now. However I tend to agree with you that 'X' should use "0X" prefix and then it makes perfect sense to have 'B' specifier (and binu).

[gcflymoto]

I don't see that. For me "{:#X}" results in 0X and "{:#x}" results in 0x .. maybe some tests for this would be a good idea, be clearer, and show more examples of formatting.

On Thursday, November 14, 2013 11:05 AM, vitaut notifications@github.com wrote:

I don't have anything against having 'B' specifier, but I think this needs to be consistent with 'X'. Currently 'X' uses "0x" prefix and that's one of the reasons I removed 'B' for now. However I tend to agree with you that 'X' should use "0X" prefix and then it makes perfect sense to have 'B' specifier (and binu).
—
Reply to this email directly or view it on GitHub.

[vitaut]

You are right. I have probably misinterpreted one of the test cases. Adding tests and documentation sounds like a good idea, I've added issue #5 to track this. Will restore 'B' and binu then.

[gcflymoto]

Great! Thanks. I'll continue sending the pull requests.. it would have been tough to maintain two different branches for the long term.

On Thursday, November 14, 2013 12:04 PM, vitaut notifications@github.com wrote:

You are right. I have probably misinterpreted one of the test cases. Adding tests and documentation sounds like a good idea, I've added issue #5 to track this. Will restore 'B' and binu then.
—
Reply to this email directly or view it on GitHub.