-
-
Notifications
You must be signed in to change notification settings - Fork 705
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
add std.conv.hexData() #7211
add std.conv.hexData() #7211
Conversation
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#7211" |
std/conv.d
Outdated
if (digits) | ||
append(u); | ||
|
||
return cast(immutable)result[0 .. i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention is to use assumeUnique
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted this function to not have dependencies on anything but the gc.
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line space will suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use two lines to separate one function from the next unrelated one.
The buildkite error is:
which has nothing to do with this PR. |
Yeah the druntime benchmark check uses a wrong version of phobos. |
@wilzbach so what's the fix? |
Nothing, as Buildkite is not required to pass to pull. You should double-check that the benchmark is the only failure though. |
enum a4 = hexData("f"); | ||
assert(a4 == [0xF]); | ||
enum a5 = hexData(" f,"); | ||
assert(a5 == [0xF]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why support odd number of digits? This is probably going to almost always be an error or typo.
enum a5 = hexData(" f,"); | ||
assert(a5 == [0xF]); | ||
enum a6 = hexData("0BF "); | ||
assert(a6 == [0x0B, 0x0F]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should probably be some tests to test the behavior with invalid inputs. Speaking of which, they seem to be ignored? Combined with the decision to support one-digit characters, this seems like a bad idea, e.g. in "54 32 1O"
the last character is the letter O.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
G-Z should be an error, that's bug prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As currently specified there are no invalid inputs to this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Please mind @CyberShadow 's review
result.length = (s.length + 1) / 2; | ||
size_t i; | ||
|
||
void append(ubyte u) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a bit much, it's called from three places
@WalterBright a rebase will most likely fix the buildkite error and then the autotester will be green. However, you still need to address @CyberShadow 's review. |
Hex string were added back into the language, there is no need for this. |
This is an alternative to
hexString
based on usage feedback from Dunnhumby.wstring
anddstring
are not supported, as it is hard to imagine a compelling use case for themubyte[]
is returned instead ofstring
as it only makes sense for hex data to initializeubyte
arrays, not stringsimmutable
so it can be used to initialize global tables with need for castingx
string core language featureIt is designed to work with dlang/DIPs#177 although that is not required for
hexData()
to be useful.