Skip to content
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 issue 9591 - Allow Instantiate template to be public #5739

Merged
merged 2 commits into from Sep 20, 2017

Conversation

schveiguy
Copy link
Member

This solves the problem in the referenced issue. I've added the solution as a documented unit test for Instantiate.

of how Instantiate + ApplyRight can be used to solve the problem.
@schveiguy
Copy link
Member Author

ping @wilzbach I simply reused my branch and opened a new PR, but dlang-bot didn't notice?

@wilzbach
Copy link
Member

ping @wilzbach I simply reused my branch and opened a new PR, but dlang-bot didn't notice?

Don't worry about it. The essential things are run in a loop and run every five minutes. It's probably an issue with the GitHub API, but I am currently on the way for a week long vacation, so I sadly can't check the logs to be sure. Thanks for the ping!

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @schveiguy!

Bugzilla references

Auto-close Bugzilla Description
9591 std.typetuple.staticApplyMap

@schveiguy
Copy link
Member Author

Ah, @wilzbach it's because I squashed the original branch, so must be dlang-bot didn't think there were changes (or github didn't notify?).

Anyway, seems good now, I had to fix the docs a bit and the bot showed up.

@atilaneves atilaneves merged commit efae085 into dlang:master Sep 20, 2017
@schveiguy schveiguy deleted the fix9591 branch September 20, 2017 10:27
@wilzbach
Copy link
Member

Shouldn't this have been formally approved by @andralex? (after all it's a "new" public symbol)

@atilaneves
Copy link
Contributor

@wilzbach Good question. I'm new around these parts.

@wilzbach
Copy link
Member

@wilzbach Good question. I'm new around these parts.

No worries - it was a rhetorical one ;-)
Every new public symbol needs to be approved by @andralex
It's probably not a big deal for this one, maybe quickly pinging him on slack is enough?

@schveiguy
Copy link
Member Author

It's newly public, but not new. I don't speak for Andrei, but I think the naming of the symbol is correct -- I can't imagine a better name is possible. But the question to answer is, does this solve a big enough problem such that it's worthy of a new symbol? Given the problem it solves in the context of staticMap, I think it is. It's also needed any time you are finding templates inside an AliasSeq that you need to instantiate. Clearly it was needed inside std.meta as well.

@andralex
Copy link
Member

I'm not thrilled about this. A compelling example should be found, if not I think we shouldn't add this kind of subtle artifacts of very limited usefulness.

@schveiguy
Copy link
Member Author

This is as essential as AliasSeq. Before it existed (as TypeTuple before), one had to always make a simple declaration in their file where they use it. So much easier when everyone uses the same name and meaning.

Instantiate is the same thing -- it's obvious, easy, but something you just have to declare somewhere, so it might as well go in a central place.

I can't think of a more compelling example besides trying to apply a bunch of similar templates to the same parameters.

All you have to do is look at all the simple templates in this file to see things that are easy to declare, but are needed many places, so this module collects them. AliasSeq, Alias, templateNot, etc. Many of the things in here are one-liners.

Also note that Instantiate was already used internally, so I didn't add anything. I just moved it to a public area and documented it.

@andralex
Copy link
Member

@schveiguy what would a good standalone example not necessitating much additional information?

@dnadlinger
Copy link
Member

dnadlinger commented Sep 20, 2017

@andralex: It crops up all over the place in "higher-order template" code, see e.g. std.meta.template{And, Or} or the example Steve added (although the latter looks a bit contrived). The usefulness is only limited in so far as that of the rest of std.meta – with the exception of AliasSeq – is as well.

Yes, it is a trivial artefact. But if a typical user encounters the problem, they would find the solution somewhat non-obvious. Hence, I'd contend that it is useful for this to be discussed in the std.meta documentation. The most straightforward way to do this is by simply making the private copy public.

@andralex
Copy link
Member

Thanks for the due diligence @klickverbot!

@dnadlinger
Copy link
Member

(In an ideal world, I'd prefer fixing that grammar limitation and adding an inline template "lambda" syntax to this, though.)

@jmdavis
Copy link
Member

jmdavis commented Sep 21, 2017

Template lambdas would be awesome. The more you do template metaprogramming, the more painful it seems that they aren't there. I have no idea what a reasonable syntax for them would be though, which is part of why I've never really tried to push the idea.

@ntrel
Copy link
Contributor

ntrel commented Oct 18, 2017

This is missing a changelog entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants