-
-
Notifications
You must be signed in to change notification settings - Fork 706
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
Fix issues 20301 and 20302 #7231
Conversation
Make std.functional.memoize accept functions whose return type or parameters are head const.
|
Thanks for your pull request, @ZombineDev! Bugzilla references
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 "stable + phobos#7231" |
22c1247 to
fdaccee
Compare
Because third-party projects already depend on this. See e.g. gecko0307/dagon: https://buildkite.com/dlang/phobos/builds/2351#71be0264-16e9-4f8a-ab79-0af45a114741
|
I'll drop commit df58d90 as it breaks users that assume |
fdaccee to
27c43b7
Compare
| if (isSomeString!(S)) | ||
| { | ||
| import std.array : appender; | ||
| import std.functional : memoize; | ||
| enum cacheSize = 8; //TODO: invent nice interface to control regex caching | ||
| S pat; | ||
| const(C)[] pat; |
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.
If the only reason to introduce C is this, maybe ElemType would be better ?
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 don't think so (maybe I'm missing something?), because:
regexImplsupports only string types (not ranges).ElemType(I assume you meanstd.range.primitives.ElementType) will returndchar, givenT = string, orT = wstring. In case we're called with such narrow strings, we would need to re-encode them to dchar[], which would be the type ofpat.const-ness. Changing the parameter fromS[] patternstoconst S[] patternsis not strictly necessary here (but it is forregexImpl), but by doing this I'm saving on distinct template instances. Before forS = char[],S = const(char)[]andS = const(char[])(ditto forimmutable) we would get different template instances. Now there's only one.
So, with this change in mind, whenregex(const S[] patterns, ..)is called with achar[][],patternsis deduced to beconst(char[][]), whileSbecomeschar[]and we get the following error:(Because in the lineError: cannot implicitly convert expression patterns[0] of type const(char[]) to char[]pat = patterns[0],patisS = char[]andpatterns[0]isconst(char[]).)
Additionally, I can't definepatasconst Sas then I won't be able to assign to it.
So what I need is the char type, so I can definepatas tail-const.- Using the template specialization syntax
S : C[], Cis much shorter and readable thenElementEncodingType(fromstd.range.primitives). It's probably also slightly faster to compile.
|
|
||
| static ReturnType!fun[Tuple!Args] memo; | ||
| static Unqual!(ReturnType!fun)[Tuple!Args] memo; |
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 Unqual really shouldn't be necessary (in a perfect world), but I guess the compiler can't recognize that.
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.
Given const(int) fun(int a, int b);, typeof(memoize!fun.memo) becomes: ReturnType!fun[Tuple!Args] -> const(int)!fun[Tuple!(int, int)]. Later on, for the line memo[t] = fun(args) you get:
Error: cannot modify const expression memo[t]
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.
Yep, but since this is insert-only, it should work.
However, it would take the compiler to recognize insert-only path (e.g. by handling require ?) to make this work.
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 know, I thought as well and I even tried using require and without Unqual change, but it has this line inside, which would fail because *p is const(int).
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.
That's said, I think it's a would be a good enhancement request.
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.
Unqual + emplace should work there, but I think that change is better left for the master branch, while this PR is targeting stable.
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.
Just a couple nits, oLGTM
27c43b7 to
b923f74
Compare
|
What about https://issues.dlang.org/show_bug.cgi?id=20300 ? |
|
Hi @trikko, I didn't see your bug report so it's a nice coincidence that yours and mine have consecutive Bugzilla numbers. I'll take a look if it's something easy to fix. |
|
@trikko I took a stab at implementing it and the good news is that it's definitely doable - see https://run.dlang.io/gist/ZombineDev/677cea7195209dec2e2b393bcf94ae24?compiler=dmd. However it wouldn't be trivial to turn my code into a proper Phobos pull request for the limited time I have today, so I'll leave it for a future PR. |
|
Too bad (: |
|
It looks like @dlang-bot is offline... |
This PR includes the following commits:
Make
std.functional.memoizeaccept functions whose return types or parameters are head const.[refactor]: Simplify memoize(alias fun) implementationUseobject.requireto save one hash table lookup[style] Consolidate imports