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 experimental refraction module #10
Conversation
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.
Wow. This is amazing! And it was quite easy to read as well!
Of course I have a number of suggestions/questions/etc :)
And one overall request is if you can please try and fix up the formatting a bit. I can go and proper clean it up later but at the very least can you use OTBS brace styles and 4-space indentations as is in all other source files?
(I should probably add dfmt_brace_style=obts
to the .editorconfig
...
body_; | ||
} | ||
|
||
string[] parameterListMixtureArray() |
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.
Do all these other mixtures have to be public?
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.
Do all these other mixtures have to be public?
Yes. It makes it possible to add parameters, combine parameter lists, etc.
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.
Are they used in any of your usecases outside the library? They seem to be mostly one-liners (except for the attributes one).
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.
Are they used in any of your usecases outside the library? They seem to be mostly one-liners (except for the attributes one).
In openmethods I need to edit the parameter themselves. I use argumentMixture
in openmethods and argumentMixtureArray
in my now obsolete ducktyping project. In the latter case, I need the array variant because I need to add arguments. As for parameterListMixtureArray
, I think that omitting it would make the API incomplete and irregular. It's only coincidental that it is also used to implement another public 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.
I think functions that are obvious and can be implemented as a one liner generally should be part of a public API except for in 2 situations: 1) if they are used a lot and 2) if they provide some kind of portability (across compilers, platforms, etc). I'm not sure in this case though.
In this case the only one that I think is actually non-trivial is attributeMixtureArray
, but adding that and not the others would make the API, as you say, irregular. So I propose a property function named attributesAsStrings
or attributeStrings
that return the attributes as string[]
and cut out the others.
Additionally, it feels a bit off that a mixture is not mixable. I'd think that anything with mixture at the end can be used as a mixin, but I don't think you can mixin argumentMixture anywhere no?
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.
In this case the only one that I think is actually non-trivial is
attributeMixtureArray
, but adding that and not the others would make the API, as you say, irregular. So I propose a property function namedattributesAsStrings
orattributeStrings
that return the attributes asstring[]
and cut out the others.
Ah, I disagree...you see, I didn't dream up this API out of context. When I set out to support function attributes and parameter storage classes in openmethods (in response to jll63/openmethods.d#13 - note that it stayed opened for 2 years), the code that generates functions went through many iterations. At one point all the generated code was created by string mixins, themselves created via format
. Essentially all the compile-time part of openmethods had turned into an ugly bunch of C-style macros. Unfortunately, I was not aware of Adam's Tip of teh Week back then.
I started experimenting with string interpolation, and that helped a little. Then I came up with a FunctionBuilder
. It helped more, but still, what emerged was that sometimes I needed the arguments as a string, and sometimes as an array (the latter, typically, when I needed to add an argument at the front of the list). The code was better, but still difficult to read because it juggled strings around pretty much like the format
version.
So I added the helpers (again it went though many iterations, some of them visible in openmenthod's master branch) with the goal of making the generation code as readable as possible. At that point FuinctionBuilder
had become a sort of DSL (and bear in mind that the goal is not to replicate functions, but to transform them easily).
At that point these was a possibility that FunctionBuilder
was in fact tailored to openmethods and was not generally useful, but then I implemented my (unpublished, obsolete) ducktyping library, and it appeared that several of these partial mixture, returning either a string or an array, were useful in the second library as well. IOW, if these functions had not been in the (then named) FunctionBuilder
, I would have had to duplicate them in openmethods and ducktyping.
As a counter-example, this code from ducktyping:
static auto makePointer(Function fun)
{
auto name = fun.name;
return fun
.removeMutabilityAttributes
.insertParameters(
0, Parameter(
fun.copyMutabilityAttribute("void*")))
.setName("function")
.setBody("")
.mixture ~ " %s%d;".format(name, fun.index);
}
struct Vtbl
{
mixin(refract!(Interface, "Interface").map!makePointer.join("\n"));
}
...uses two helper functions:
string copyMutabilityAttribute(Function fun, string type)
{
with (FunctionAttribute) {
return fun.attributes & const_ ? "const " ~ type
: fun.attributes & immutable_ ? "immutable " ~ type
: type;
}
}
private auto removeMutabilityAttributes(immutable(Function) fun)
{
return fun.setAttributes(
fun.attributes & ~(FunctionAttribute.const_ | FunctionAttribute.immutable_));
}
These are clearly ducktyping-specific, and would have no use in openmethods. OTOH, argumentMixture
is used in both, and I predict that most use cases of refract
will need it as well.
All that to say, the API is not a case of armchair design. It grew up from considering the converging and diverging needs of two libraries.
Additionally, it feels a bit off that a mixture is not mixable. I'd think that anything with mixture at the end can be used as a mixin, but I don't think you can mixin argumentMixture anywhere no?
The "mixture" stem also came after many iterations. Initially, they were called asString
(following your convention in bolts) and asStringArray
. But then I felt that these strings are created following strict constraints, with one ultimate goal: land inside a string mixin. So I renamed them to asMixin
and asMixinArray
. But then those strings were not complete mixins - as you justly point out -, more like fragments of mixins. Thus they became asFragment
and asFragmentArray
(this time inspired by Beta). But that lost the focus on D's mixins. Eventually I came up with mixture. A mixture is not a mixin, but a bit of mixin that, mixed with other mixtures, eventually builds into a string mixin.
In the near future I hope to popularize Adam's tip, and promote the word "mixture" to mean "bit of source code that, when string-mixed-in, will work correctly even across modules, like Adam initially explained".
Also, thinking about the API a bit. I realize this is a classical builder. So it's a
If we don't call
|
Actually at some point I called it In the end, I would not object to Now that I can discuss this publicly, maybe we should ask opinions in the forum.
I am pretty sure I suggested that in an email a while ago. Yeah
The name
Yes I do that in I will go through the code comments during the week-end. |
This is in fact my preferred style. I put the curly braces on their own line because that's what I saw in Phobos.
Sure. 2 was the emacs default. |
I think I addressed all your points (except the few I feel we need to discuss further) in the "review #1" commit. |
index = the index of the function in the set of overloaded function called `name` | ||
*/ | ||
|
||
Function refract(alias Scope, string localName, string name, uint index)() |
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.
Have you found use for this overload of refract
in your use cases (aka do you need this for open methods or any other library?)
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.
Have you found use for this overload of
refract
in your use cases (aka do you need this for open methods or any other library?)
My ducktyping lib uses the 3rd overload:
static auto makePointer(Function fun)
{
auto name = fun.name;
return fun
.removeMutabilityAttributes
.insertParameters(
0, Parameter(
fun.copyMutabilityAttribute("void*")))
.setName("function")
.setBody("")
.mixture ~ " %s%d;".format(name, fun.index);
}
struct Vtbl
{
mixin(refract!(Interface, "Interface").map!makePointer.join("\n"));
}
This shows that in practice, the index is often needed - here it is to make unique identifiers for pointers to overloaded functions.
Currently I don't use overload #2 but I think I did at one point, and I think that it will be needed at times, because the only way to form a symbolic reference to a function involves the module, the name and the index in the overload set.
Probably a FunctionRef(alias Module, string name, int index)
would be a useful addition to bolts, along with a variant that takes a module, a name, and a signature, e.g.:
module h2g2;
struct FunctionRef(alias _module, string _name, int _index) {
alias Module = _module;
enum name = _name;
enum index = _index;
alias Alias = __traits(getOverloads, Module, name)[index];
}
struct FunctionRef(alias _module, string _name, Pattern) {
static foreach (member; __traits(allMembers, _module)) {
static if (member == _name) {
static foreach (i, fun; __traits(getOverloads, _module, _name)) {
static if (!__traits(compiles, { enum _ = index; })) {
static if (is(typeof(&fun) == typeof(Pattern()))) {
alias Module = _module;
enum name = _name;
enum index = i;
alias Alias = fun;
}
}
}
}
}
}
string answer() { return "forty-two"; }
int answer() { return 42; }
alias f0 = FunctionRef!(h2g2, "answer", 0);
static assert (f0.Alias() == "forty-two");
alias f1 = FunctionRef!(h2g2, "answer", int function());
static assert (f1.Alias() == 42);
static assert (f1.index == 1);
Well, all that to say, addressing a function in presence of overloads is messy, and that's why I speculate that refract(alias Scope, string localName, string name, uint index)()
will be useful in some cases, even though I don't use it anywhere right now (or maybe in a branch where I try to make openmethods work with templates, and that is a headache).
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.
static if (!__traits(compiles, { enum _ = index; })) ...
Huh ... neat trick!
I'm not sure that FunctionRef!(h2g2, "answer", 0);
is better than __traits(getOverloads, h2g2, "answer")[0];
but that pattern one is cool, is it practical?
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.
On the overloads. I feel a bit uneasy about the third. I'll try and explain:
The name of the utility is refraction and it makes sense when you want to perform refraction on a "thing". With the first overload this thing is a function. One the second overload, it's a function with in an overload set inside a scope. Which I also guess would be needed but for the third I'm having trouble as you're not refacting the interface. You're returning an array of the interface's member functions that match a certain pattern. It's a get+filter.
It seems the mixin you posted as an example for overload 3 should be: extractFunctionsFrom!Interface.map...
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.
static if (!__traits(compiles, { enum _ = index; })) ...
Huh ... neat trick!
Inspired by something I saw in Phobos ;-)
I'm not sure that
FunctionRef!(h2g2, "answer", 0);
is better than__traits(getOverloads, h2g2, "answer")[0];
but that pattern one is cool, is it practical?
I think that it's more readable, also we would add static assert
s to better report cases where the function does not exist, or the overload index is out of range. The patterned version, I used in some of my many failed attempts of blending open methods with templates. But I have seen the question "how do I create an alias to an overloaded function" come up several times in the forums.
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.
On the overloads. I feel a bit uneasy about the third. I'll try and explain:
The name of the utility is refraction and it makes sense when you want to perform refraction on a "thing". With the first overload this thing is a function. One the second overload, it's a function with in an overload set inside a scope. Which I also guess would be needed
It turns out that neither openmethods nor ducktyping use this overload. I'm fine with removing (or privatizing) it.
but for the third I'm having trouble as you're not refacting the interface. You're returning an array of the interface's member functions that match a certain pattern. It's a get+filter.
Yeah maybe refract
is getting a bit overloaded, semantically.
I considered the possibility that we add aggregate or module refraction later (actually a previous version, that used templates instead of ctfe objects, had it). Perhaps we should stick to the principle that refract
always returns an object representing its first argument. We could have:
module mymodule;
enum mod = refract!mymodule; // type: Module!mod
enum modFunctions = mod.functions!(filter); // type: Function[]
interface Foo { ... }
enum interfaceFunctions = refract!Foo.functions; // type: Function[]
In the meantime, it turns out that refracting all the functions in an aggregate is very useful. ducktyping does it in several places. Thinking about it, it would be useful if we could tell it to store a flat function index or an overload index.
In the light of this, I suggest two options:
- I rename overload hasProperty crashes if T is a pointer #3 to
extractFunctionsFrom
or (IMO better)refractFunctionsFrom
(and remove Fix typo #2) - I write a minimal implementation of module and aggregate refraction, just what we need to host the en-masse refraction of functions at this point (and we'll beef them up later).
What do you think?
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.
But I have seen the question "how do I create an alias to an overloaded function" come up several times in the forums.
Yes I believe I have as well.
Something in the heart of bolts would be appropriate here indeed I believe. AliasOf
? or should std.meta.Alias
be extended?
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 bolts is to Phobos what Boost is to the C++ std lib, let's put it there, so we can showcase it, then try to push it to Phobos.
What about bolts.members.overloadAlias
?
Apologies for the time delay again :) It'll be faster going forward as my personal crunch has calmed a bit. I think I've addressed everything with some more suggestions and questions. Agreed on the squash at the end. |
Thanks! I'll work on this during the long weekend. |
All processed, back to you. |
index = the index of the function in the set of overloaded function called `name` | ||
*/ | ||
|
||
Function refract(alias Scope, string localName, string name, uint index)() |
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.
static if (!__traits(compiles, { enum _ = index; })) ...
Huh ... neat trick!
I'm not sure that FunctionRef!(h2g2, "answer", 0);
is better than __traits(getOverloads, h2g2, "answer")[0];
but that pattern one is cool, is it practical?
localName = a string that represents `fun` in the caller's context | ||
*/ | ||
|
||
Function refract(alias fun, string localName)() |
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 not have this one take an overload index as well?
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 not have this one take an overload index as well?
Indeed why not, and give it a default value of -1:
Function refract(alias fun, string localName, uint index = -1)()
It would make it possible to write refract!(foo, "foo", 2)
directly, instead of refract!(foo, "foo").withIndex(2)
. In fact if we do this, we can remove withIndex
altogether, as your remark above ("What's the use of changing the index then?") suggests. Either the user needs to store a flat, module-global index, or an overload set local index, but once it's set, there's no changing it, because functions don't wander in modules.
Yeah, thinking about it, it looks better like this...
index = the index of the function in the set of overloaded function called `name` | ||
*/ | ||
|
||
Function refract(alias Scope, string localName, string name, uint index)() |
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.
On the overloads. I feel a bit uneasy about the third. I'll try and explain:
The name of the utility is refraction and it makes sense when you want to perform refraction on a "thing". With the first overload this thing is a function. One the second overload, it's a function with in an overload set inside a scope. Which I also guess would be needed but for the third I'm having trouble as you're not refacting the interface. You're returning an array of the interface's member functions that match a certain pattern. It's a get+filter.
It seems the mixin you posted as an example for overload 3 should be: extractFunctionsFrom!Interface.map...
🎾 in your court 🤓 |
Not pushing a commit yet, waiting for your feedback on my replies. |
I agree with all your replies 👍 Let's make those changes and get this in and start iterating ! I'll play the waiting game on the mixtures that are not immediately mixable though ;) On the overloads, I'd agree with renaming overload 3 and removing 2 while adding index to 1. As for name, i agree |
I accidentally resolved a few things without the extra commit because I didn't wait to read till your last comment :p. And I don't remember what they were so if you can figure them out then great, if they fall through and I don't catch them then we can always fix later! |
I'm also unsure about the body/declaration thing because it feels a bit surprising that if the identity refraction is a decleration and not a mixiable function. But also we can iterate here if necessary |
Yeah, |
There's no good default really. Initially the body was initialized to . Then I switched to the semicolon and liked it. The mixture is syntactically correct from the start. Examples and unit tests are a bit simpler. It's even useful in real code, somewhere I use it to declare a pointer to a function - |
All done on my side, I think. You can squash-merge if it's good with you... |
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.
Some very minor documentation stuff, one ACL thing, and since the doc stuff needs to be done, a reminder on a naming thing.
But LGTM!
alias ReturnType = std.traits.ReturnType; | ||
alias Parameters = std.traits.Parameters; | ||
|
||
template ParameterAttribute(alias F, int i, int j) { |
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.
UNdocumented public name
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.
Also moved to bottom of module so it does not appear before the main stuff in the doc.
|
||
Params: | ||
fun = a function | ||
localName = a string that represents `fun` in the caller's context |
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.
index docs missing
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.
Funny github does not allow me to reply to your message below. Putting my comments here.
Doc fix. No withIndex anymore.
...and the doc of index
needed updating as well, for the 2nd refract
is now functionsOf
.
Also could still rename to overloadIndex?
So I'm pretty sure that half of the time it will be more useful to have the index of the function in the module or aggregate. For example, suppose we wanted to implement the Mock
example fully. It would be much more convenient to have the flat index. Assume for a moment that functionsOf
stores the flat index, not the overload's, in index
. We could write:
unittest {
interface GrandTour {
pure int foo();
@nogc @trusted nothrow ref int foo(out real, return ref int, lazy int);
@safe scope void bar(scope Object);
}
class Mock(Interface) : Interface {
enum functions = functionsOf!(Interface, "Interface");
int[functions.length] calls;
static foreach (Function fun; functions) {
mixin({
if (is(mixin(fun.returnType) == void)) {
return fun.withBody("{ ++calls[fun.index]; }").mixture;
} else if (fun.attributes & FunctionAttribute.ref_) {
return fun.withBody(q{{
++calls[fun.index];
static %s rv;
return rv;
}}.format(fun.returnType)).mixture;
} else {
return fun.withBody(q{{
++calls[fun.index];
return %s.init;
}}.format(fun.returnType)).mixture;
}
}());
}
}
auto mock = new Mock!GrandTour;
real x;
int i, l;
mock.foo(x, i, l++) = 1;
assert(mock.foo(x, i, l++) == 1);
assert(mock.calls[1] == 1);
assert(l == 0);
}
Of course this is a bit naive - I had to make all functions mutable. But you get the idea...
So, this morning I realized that we should store both: index
which contains the flat index, and - you'll be happy - overloadIndex
which is what we have now, renamed.
I am going to put this change in a separate commit, in case we can't agree on it.
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.
OK forget about this "other" index. While playing with this, I realized that:
- the position a function (overload) inside a scope is...a complicated concept.
__traits
returns functions by name, then by overload. - all I need is some sort of flat index, and the position of the
Function
inside the array returned byfunctionsOf
does just fine. See exemple below.
So I am going to just rename index
to overloadIndex
as you suggest :)
// dmd -i -I=~/dev/bolts/source/ -main -unittest -mixin=mixins.d refraction_demo.d
module demo;
import std.format;
import std.traits : FunctionAttribute;
import bolts.experimental.refraction;
import std.algorithm.searching;
unittest {
interface GrandTour {
pure int foo();
@nogc @trusted nothrow ref int foo(out real, return ref int, lazy int);
@safe scope void bar(scope Object);
}
class Mock(Interface) : Interface {
enum functions = functionsOf!(Interface, "Interface");
int[functions.length] calls;
static foreach (Function fun; functions) {
mixin({
auto index = functions.countUntil(fun);
if (is(mixin(fun.returnType) == void)) {
return fun.withBody("{ ++calls[%s]; }").mixture.format(index);
} else if (fun.attributes & FunctionAttribute.ref_) {
return fun.withBody(q{{
++calls[%s];
static %s rv;
return rv;
}}.format(index, fun.returnType)).mixture;
} else {
return fun.withBody(q{{
++calls[%s];
return %s.init;
}}.format(index, fun.returnType)).mixture;
}
}());
}
}
auto mock = new Mock!GrandTour;
real x;
int i, l;
mock.foo(x, i, l++) = 1;
assert(mock.foo(x, i, l++) == 1);
assert(mock.calls[1] == 1);
assert(l == 0);
}
BTW: About the localName thing. Can you maybe show a very minimal example of user code that would not work without the localName stuff? (not related to changed in PR, just for understandings sake) |
Easy, in the
The mixin expansion looks like this: // expansion at source/bolts/experimental/refraction.d(93,17)
pure @system immutable bolts.experimental.refraction.ReturnType!(foo) foo(){
return bolts.experimental.refraction.ReturnType!(foo).init;
} Which fails because in this scope, there is no I hate redundancy too. What we would need is, somehow, make |
I think I processed all your comments. I also did a few minor improvements (mostly formatting) which I put in isolated commits. Back to you... |
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.
Sweet!! 🧗♂️🚀
No description provided.