Add D string overloads to methods in StringTable#8547
Conversation
|
Thanks for your pull request and interest in making D better, @jacob-carlborg! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. 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 + dmd#8547" |
| return getValue(table[i].vptr); | ||
| } | ||
|
|
||
| inout(StringValue)* lookup(const(char)[] str) inout nothrow pure |
There was a problem hiding this comment.
I hate to nitpick, but new public functions should have full DDoc.
There was a problem hiding this comment.
I know, I know 😃.
jacob-carlborg
force-pushed
the
dstring_overlods
branch
from
b21cdbb to
ed1c43b
Compare
src/dmd/root/stringtable.d
Outdated
| * | ||
| * Returns: the string's associated value, or `null` if the string doesn't | ||
| * exist in the string table | ||
| */ |
There was a problem hiding this comment.
FWIW the DStyle we use for new symbols at Druntime/Phobos says:
Documentation comments should not have leading stars on each line.
There was a problem hiding this comment.
It looks so weird. I have a feeling that most of the code out there is not using this style.
jacob-carlborg
force-pushed
the
dstring_overlods
branch
from
ed1c43b to
90cae21
Compare
|
|
||
| Params: | ||
| s = the string to look up | ||
| length = the length of $(D_PARAM s) |
There was a problem hiding this comment.
Technically str would need to be documented too (as it gets dittoed into this documentation.)
There was a problem hiding this comment.
We need a better way to inherit documentation that ditto. Fixed.
This will help reduce the surface of where C strings and pointer length pairs are used.
jacob-carlborg
force-pushed
the
dstring_overlods
branch
from
90cae21 to
39be8a0
Compare
|
Do these new methods need to be placed at after all the C++ methods sine they cannot appear in the C++ header? |
Yeah, that may be nice. I won't hold up this PR on it though, and it can always be done as a followup. |
|
Semaphore CI seems stuck, so I'm closing and re-opening. |
Actually, since these functions are not virtual and don't occupy a slot in the vtable the order doesn't matter [1]. In that case I think it makes more sense to have the overloads next to each other. [1] #8550 (comment) |
4 participants
This will help reduce the surface of where C strings and pointer length pairs are used.