[Nemerle 0001072]: Allow try inside expressions
feed...@nemerle.org
The following issue has been RESOLVED.
======================================================================
<http://nemerle.org/bugs/view.php?id=1072>
======================================================================
Reported By: steffen
Assigned To: divan
======================================================================
Project: Nemerle
Issue ID: 1072
Category: Compiler
Reproducibility: always
Severity: feature
Priority: normal
Status: resolved
Resolution: fixed
Fixed in Version:
======================================================================
Date Submitted: 12-29-2007 22:38 CET
Last Modified: 12-09-2008 23:27 CET
======================================================================
Summary: Allow try inside expressions
Description:
I've written a patch to allow try inside expressions.
The idea is to track for every TExpr whether it needs an empty stack.
When a function call is made where one of the arguments needs an empty
stack all arguments before this argument are saved to local variables.
======================================================================
Relationships ID Summary
----------------------------------------------------------------------
related to 0000979 yielding: don't always transform var-&g...
related to 0000259 cannot strore result of a try block
related to 0001077 ICE in LocalValue.n (201)
======================================================================
----------------------------------------------------------------------
nazgul - 12-29-07 23:28
----------------------------------------------------------------------
This is cool!
Some comments:
+ def nassigns = assigns.Map ((_, e) => NeedsEmptyStack (e));
+ nassigns.Contains (true)
You should just use Find or similar method, which checks if function
returns true on any element.
+ def nobj = NeedsEmptyStack (obj);
+ def nargs = args.Map (NeedsEmptyStack);
+ nobj || nargs.Contains (true)
As above, there are several places to fix it.
+ emit_exprs (parms.Map (parm => parm.expr));
Can also be written parms.Map(_.expr)
+ emit_exprs (() => (), exprs);
I would rather use null here, which is maybe less elegant, but will be
faster (no need to create function instance on each call of this method)
+ private emit_exprs (before_first_value : void->void, exprs :
list[TExpr]) : void
We usually have function as last parameter. Also, I think you could use
default value (null) here and do not have a separate method for just
exprs.
Did you consider splitting emit_exprs into two methods, one which would
emit the values and other to emit pushing those values? This way it would
be the caller's responsibility to run "before_first_value", which is IMHO
cleaner when you read places, which require this "in-between" processing.
- unless (ctx %&& Context.AllowTry)
This this flag ever used now? Probably it is (haven't check the source
code), but if not, then we can remove its initialization.
I think we should run all the tests for it on MS.NET before committing - I
will do it in next week.
----------------------------------------------------------------------
steffen - 12-30-07 00:41
----------------------------------------------------------------------
> + def nassigns = assigns.Map ((_, e) => NeedsEmptyStack (e));
> + nassigns.Contains (true)
>
> You should just use Find or similar method, which checks if function
returns
> true on any element.
That won't work because NeedsEmptyStack has side effects (it will compute
the NeedsEmptyStack flag for all expressions in e) and Find (or Exists)
will stop when the first "true" value is found.
> + emit_exprs (parms.Map (parm => parm.expr));
>
> Can also be written parms.Map(_.expr)
I've changed that.
> + emit_exprs (() => (), exprs);
>
> I would rather use null here, which is maybe less elegant, but will be
> faster (no need to create function instance on each call of this
method)
Shouldn't the instance be cached? It isn't right now, but the function
doensn't use any local variables or stuff like that.
> Did you consider splitting emit_exprs into two methods, one which
> would emit the values and other to emit pushing those values? This
> way it would be the caller's responsibility to run
"before_first_value",
> which is IMHO cleaner when you read places, which require this
> "in-between" processing.
I've done this now (new patch is attached).
> - unless (ctx %&& Context.AllowTry)
>
> This this flag ever used now? Probably it is (haven't check the source
code),
> but if not, then we can remove its initialization.
It is still used, but it probably can be removed. (There is still the code
for not inlining local functions with try-blocks in an expression context,
which isn't needed anymore now, and also should be removed.)
----------------------------------------------------------------------
divan - 12-30-07 12:13
----------------------------------------------------------------------
On win64 all tests are ok.
But some of generated executables contain errors according to PEVerify
(I'll have a closer look on errors).
Btw, why don't we add PEVerify checks for unittests?
----------------------------------------------------------------------
divan - 12-30-07 12:59
----------------------------------------------------------------------
One problem is assigning to fields, object is pushed in stack before try
block begins, so stack isn't empty.
class A {
x : int;
this () {
x = try {1} finally {2} //bad IL
}
}
So to allow this we maybe need to calculate obj, store it in local,
calculate val, push obj, push val, store val in obj.field. And add
optimization: if obj calculation surely doesn't throw then just move try
block start before (calculating and) pushing obj..
Anyway there are many things to think on like
obj.obj_field.obj_field_field=try{}.
Another one is try blocks in ctors before base call, PEVerify says it's
not allowed, but i haven't yet found it in specification. If it's true
then it's a separate bug:
class A {
this() {
try {1} finally {2}; //bad IL
base()
}
}
And in case it's not allowed things like base(try{}), this(try{})
(try-expressions.n) should be banned, with the exception of structs (they
don't have base calls and restrictions like this).
----------------------------------------------------------------------
steffen - 12-30-07 18:28
----------------------------------------------------------------------
I missed all the assignments (to fields, to ref/out parameters and to
arrays). Should work with the new patch.
> And add optimization: if obj calculation surely doesn't throw then just
move
> try block start before (calculating and) pushing obj..
> Anyway there are many things to think on like
> obj.obj_field.obj_field_field=try{}.
We also would have to check whether evaluation obj has side effects, so
this would be rather difficult.
If obj is "this" this has to be done anyway (because I don't think we may
save a reference to "this" in a local variable before calling the base
ctor).
As for the try before the base ctor call I haven't found anything in the
specification either, so I think PEVerfiy is wrong.
----------------------------------------------------------------------
divan - 01-25-08 18:38
----------------------------------------------------------------------
Sorry about such a big delay.
Last version passed all tests on my win64, except one: afair
positive/matching.n, ncc crashed with stack overflow. Manually starting it
with a bit increased stack helped. But I failed add command line stack
option to this test.. maybe something is broken in my environment. I'll
have a look at this problem again and hopefully commit this patch soon.
----------------------------------------------------------------------
divan - 12-09-08 23:27
----------------------------------------------------------------------
Applied patch on r8162, sorry for a delay again :/
Issue History
Date Modified Username Field Change
======================================================================
12-29-07 22:38 steffen New Issue
12-29-07 22:38 steffen File Added: try_expr.patch
12-29-07 23:28 nazgul Note Added: 0002022
12-30-07 00:41 steffen Note Added: 0002023
12-30-07 00:41 steffen File Added: try_expr2.patch
12-30-07 12:11 divan Relationship added related to 0000979
12-30-07 12:13 divan Note Added: 0002024
12-30-07 12:56 divan Note Added: 0002025
12-30-07 12:59 divan Note Edited: 0002025
12-30-07 18:18 steffen File Added: try_expr3.patch
12-30-07 18:28 steffen Note Added: 0002026
01-13-08 21:33 divan Issue Monitored: divan
01-25-08 18:38 divan Note Added: 0002031
02-24-08 17:52 KLiss Relationship added related to 0001077
03-08-08 09:09 divan Status new => assigned
03-08-08 09:09 divan Assigned To => divan
05-06-08 22:20 divan Relationship added related to 0000259
12-09-08 23:27 divan Note Added: 0002145
12-09-08 23:27 divan Status assigned => resolved
12-09-08 23:27 divan Resolution open => fixed
12-09-08 23:27 divan Description Updated
======================================================================