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.
The convention is to use assumeUnique
There was a problem hiding this comment.
I wanted this function to not have dependencies on anything but the gc.
|
|
||
|
|
There was a problem hiding this comment.
One line space will suffice.
There was a problem hiding this comment.
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.
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.
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.
G-Z should be an error, that's bug prone.
There was a problem hiding this comment.
As currently specified there are no invalid inputs to this function.
andralex
left a comment
andralex
left a comment
There was a problem hiding this comment.
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.
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. |
9 participants
This is an alternative to
hexStringbased on usage feedback from Dunnhumby.wstringanddstringare not supported, as it is hard to imagine a compelling use case for themubyte[]is returned instead ofstringas it only makes sense for hex data to initializeubytearrays, not stringsimmutableso it can be used to initialize global tables with need for castingxstring core language featureIt is designed to work with dlang/DIPs#177 although that is not required for
hexData()to be useful.