Skip to content

Fix Issue 17249: Added BigInt.getDigit#5987

Merged
andralex merged 2 commits intodlang:masterfrom
JackStouffer:issue17249
Jan 25, 2018
Merged

Fix Issue 17249: Added BigInt.getDigit#5987
andralex merged 2 commits intodlang:masterfrom
JackStouffer:issue17249

Conversation

@JackStouffer
Copy link
Contributor

Get the elements of the underlying representation. This would also be useful in my stdxdecimal library.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @JackStouffer!

Bugzilla references

Auto-close Bugzilla Description
17249 Make BigInt data visible (not modifiable!)

std/bigint.d Outdated
* Returns:
* The nth `ulong` in the representation of this `BigInt`.
*/
ulong ulongDigit(int n) pure nothrow const @safe @nogc
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using only one functions?
Something lilke:

Type digit(Type)(size_t n)

Usage:

BigInt(20).digit!ulong(0).

@JackStouffer
Copy link
Contributor Author

@wilzbach done

std/bigint.d Outdated
* The nth `ulong` in the representation of this `BigInt`.
*/
T getDigit(T = ulong)(size_t n) const
if (is(T == ulong) || is(T == uint))
Copy link
Contributor

Choose a reason for hiding this comment

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

Constraint indentation should be on the same level... I thought there was a test for this :/

Copy link
Contributor

Choose a reason for hiding this comment

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

dlang-community/D-Scanner#450 - it just never made it into DScanner.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Nice! As usual for new public symbols this needs @andralex's approval and a changelog entry.

@wilzbach wilzbach added @andralex Review:Needs Changelog A changelog entry needs to be added to /changelog labels Jan 3, 2018
@JackStouffer JackStouffer removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Jan 3, 2018
@JackStouffer JackStouffer changed the title Fix Issue 17249: Added BigInt.ulongDigit and BigInt.uintDigit Fix Issue 17249: Added BigInt.getDigit Jan 3, 2018
}
public:

///
Copy link
Member

Choose a reason for hiding this comment

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

Was this removed because the following method wasn't intended to be public? Also, does this change somehow relate to this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Public is already set a couple of lines above. It's pretty redundant and I don't think it's a problem to include such small things if they are trivial.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.

Though I'm starting to agree with @andralex that private should be attached per declaration rather than wholesale, for better context when reading the code. But that's for another day.

@quickfur
Copy link
Member

ping @andralex

This one has been rotting here for a while. Let's move it forward!

* The nth `ulong` in the representation of this `BigInt`.
*/
T getDigit(T = ulong)(size_t n) const
if (is(T == ulong) || is(T == uint))
Copy link
Member

Choose a reason for hiding this comment

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

I'd think ushort and ubyte would also be useful, but can be added later.

@andralex andralex merged commit adee662 into dlang:master Jan 25, 2018
@WebFreak001
Copy link
Member

uh this is a bit late now, but shouldn't this also document what it does for negative BigInts?

@JackStouffer
Copy link
Contributor Author

A note could be added but the docs say it's about the representation and the representation is the same for both negative an positive numbers.

@JackStouffer JackStouffer deleted the issue17249 branch January 30, 2018 17:10
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.

6 participants