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

Process and bind subsignature before running where clauses (RT#123596) #535

Closed
wants to merge 1 commit into from

Conversation

skids
Copy link
Contributor

@skids skids commented Sep 16, 2015

It seems ordering of the code chunks only affects the unwanted behavior.

I don't know if all where clauses hit the bootstrap binder, so this
might not be a complete fix; consult some guts folks before closing ticket.

It seems ordering of the code chunks only affects the unwanted behavior.

I don't know if all where clauses hit the bootstrap binder, so this
might not be a complete fix; consult some guts folks before closing ticket.
@FROGGS
Copy link
Member

FROGGS commented Sep 16, 2015

I think that also needs to be done in the Binder.java.

@jnthn
Copy link
Member

jnthn commented Sep 16, 2015

Also to check there's consensus this is the right thing, because I'm not
convinced it is. Now you can do a where before the sub-sig so you don't go
unpacking things that don't meet constraints, and a where inside the
sub-sig that asserts things on the unpack. With this patch we'd lose being
able to do the first.

On Wed, Sep 16, 2015 at 8:25 AM, Tobias Leich notifications@github.com
wrote:

I think that also needs to be done in the Binder.java.


Reply to this email directly or view it on GitHub
#535 (comment).

@skids
Copy link
Contributor Author

skids commented Sep 16, 2015

One arguments in favor of changing it is that the subsignature appears visually before the where clause, so it is natural to expect it to work.

But jnthn's is a pretty persuasive argument in that moving the checks inside the where can get the same effective results plus the ability to prevent the inside clauses from running via an external clause.

zoffixznet added a commit that referenced this pull request Feb 16, 2017
NQP bump brings:
Raku/nqp@2017.01-78-g8371cb2...2017.01-95-g575e92b

575e92b Bump MoarVM
6f3417a Fix data race in NFA cache.
ac9a66a Regression test for bug fixed on the js backend.
1d57305 [js] Fix type conversion bug in while ... -> $cond {...}
f798b99 Test hash iterators more throughoutly.
d38a796 [js] Make hash iterators return themselves just like on other backends.
01389bc [js] Do our fake autoclose also on things that aren't static codes.
cdac54b [js] Call $$elems directly instead of using nqp.op.elems.
8a378c1 [js] Make nqp::throwpayloadlexcaller work correctly when exception handlers are used.
682011f [js] Remove accidently disabled debugging leftover.
96115eb Test the PRINTING cclass.
fd8362a [js] Implement PRINTING in cclass, improve CONTROL.
236ca67 Test the interaction between variable setup and defaults.
36030cc [js] Make defaults depending on variable setup code work.
3268b05 Add very basic nqp::seekfh documentation
b2c0b3c FH output ops return the number of bytes written
f429832 Remove no-longer-correct note...

MoarVM bump brought: MoarVM/MoarVM@2017.01-45-g2b0739d...2017.01-72-g542baec

542baec Merge pull request #537 from MasterDuke17/minor_optimization_to_MVM_string_indexing_optimized
7b2e3d9 Factor out repeated code.
5538029 Del unused var, don't call MVM_string_graphs twice
775af41 fix GC problem in string repeat op
7bd7232 Merge pull request #532 from jeffythedragonslayer/VMArray2
7a7de2f Merge pull request #535 from samcv/fixes
bf2d419 renamed header file
26a3c52 Use utf8 for unicode_db files
685af60 Merge pull request #528 from nanis/nanis-utf8-argv
8bd00b8 Avoid namespace pollution as requested by @jnthn
7ff91db Merge pull request #534 from nanis/nanis-nocygwin
8c2c627 Issue #533: MoarVM does not build on `cygwin`.
b8e8081 Add -municode only when linking moar.exe
1c77b94 Adding -municode indiscriminately to ldflags caused minilua build to fail
e197d97 Add missing #ifdef for variable initialization
3024cfb Merge branch 'master' of https://github.com/moarvm/moarvm into VMArray
66fdd71 renamed MVMArray file and constant to VMArray
0157007 Merge pull request #531 from dogbert17/master
18df52c Fix overflow on 32 bit systems in is_full_collection()
e162cc1 gcc kept complaining about the `const`s, remove them
e7c14a3 On Windows with MinGW toolchain, add -municode as late as possible
6ebfd68 gcc complained about missing const
f94085a Spelling error was hiding incorrect and unneeded declaration of _wenviron
3354962 Accidentally broke *nix builds with 8e1ae47a6e. Fix that.
8e1ae47 On Windows, populate environment hash from Unicode environment
ecca098 On Windows, create UTF-8 encoded argv upon program entry
f5bfdcd Remove attempt to transcode from ANSI to UTF-8
@JJ
Copy link
Collaborator

JJ commented May 30, 2018

Now it's got conflicts too.

@patrickbkr
Copy link
Contributor

I just submitted this to problem-solving to get consensus on the desired behaviour. Once that's clear we can close the bug report and either merge or close this PR.

@JJ
Copy link
Collaborator

JJ commented May 3, 2020

Conflicts need to be cleared first...

@JJ
Copy link
Collaborator

JJ commented May 6, 2020

Since this is already in the problem-solving repo, I'm closing this PR. It can be reopened, salvaged or a new one can be created when that's cleared.

@JJ JJ closed this May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants