std.string and std.traits: use locally-scoped selective imports and document module-scope symbols#4370
std.string and std.traits: use locally-scoped selective imports and document module-scope symbols#4370DmitryOlshansky merged 1 commit intodlang:masterfrom joakim-noah:imports
Conversation
| import std.meta;// AliasSeq, staticIndexOf | ||
| import std.range.primitives;// ElementEncodingType, ElementType, hasLength, hasSlicing, isBidirectionalRange, isForwardRange, isInputRange, isOutputRange, isRandomAccessRange | ||
| import std.traits;// isConvertibleToString, isNarrowString, isSomeChar, isSomeString, StringTypeOf, Unqual | ||
|
|
There was a problem hiding this comment.
These need to be formatted differently.
There was a problem hiding this comment.
As in those lines are too long or what?
There was a problem hiding this comment.
That, and there's no space between ; and //, and the comments should be aligned if possible.
There was a problem hiding this comment.
Why do they need to exist at all?
There was a problem hiding this comment.
That, and there's no space between ; and //, and the comments should be aligned if possible.
There's other instances without the space in phobos, but whatever, I'll add it and break up the lines. As for alignment, these aren't really comments, so shouldn't be a need.
Why do they need to exist at all?
The plan is to remove the comment slashes and turn these into selective imports, once we have the green light for module-scope selective imports in phobos, after the current transitional period to accomodate downstream users is over.
There was a problem hiding this comment.
So if I understand correctly, these imports only exist to trigger the deprecation messages?
There was a problem hiding this comment.
So if I understand correctly, these imports only exist to trigger the deprecation messages?
I don't think they'll trigger anything right now, but not sure. Martin asked that we don't make any module-scope imports selective till the transitional period is over, so I'm just listing the imported symbols for now.
Update: I believe the issue is that some users may use the transitional import flags to revert to the previous leaky selective imports in dmd, so if these phobos imports were turned into selective imports now, these symbols would then leak into other modules for those users. We're giving them time to clean up all downstream code that inadvertently relies on such leaks, after which the old behavior will be removed and we can make these selective.
|
Traits are OK, std.string I'm skeptical of. |
|
as @DmitryOlshansky ;-) split this PR into two, get the traits part merged and work on string. It's makes every bodies life easier if PR's are as focused as possible. |
|
This PR is focused- there's no difference between the types of changes made for one module versus the other- and it's already fairly small. It's not at all clear why Dmitry is skeptical of one and not the other, as he only raised two minor issues and has not responded to my responses. |
|
a nearly 300 line diff is not small. and it is not focused as you changed 2 files in a logically unrelated way. Having more focused PR's will help you get them in faster and I imagine you want that. Just saying. |
| import std.meta; | ||
| import std.range.primitives; | ||
| import std.traits; | ||
| import std.meta; // AliasSeq, staticIndexOf |
There was a problem hiding this comment.
If you know what you need from std.meta why don't you actually only import that. Having them as comments behind the actual import is IMO strange.
|
LGTM |
…ocument symbols imported at module scope, checked with ddmd
|
Updated this PR to import all free functions for arrays from Thanks for pushing on this a second time, Dmitry, let me know if there's anything else. I'll go back and fix the previous modules where this mistake might have been made too. |
|
Auto-merge toggled on |
7 participants
Checked with ddmd, as always,
also fixed extra spaces in, never mind, that's now fixed in a later PR, #4380, so removed it from this one.std.uni