[Mono-dev] [PATCH] System.Linq.Expressions

[Marek Safar]

Hello Federico,
> this is the first of a series of patches vs the System.Linq.Expressions
> namespace. This patch doesn't change a lot of things but at least adds
> some tests (they were completely missing before). Included:
>
> * Added Test/ directory, modified Makefile to build and execute them
> * Changed a couple of Expression methods to raise exceptions identicals
>   to MS ones
> * Added tests for AddExpression and ConstantExpression
> * Implemented somme missing stuff in BinaryExpression
> * The stuff in ExpressionUtils is very generic and does quite some
>   redundant checks: I started splitting the stuff there into more   
>   "specific" methods that should be both understandable and fast.
>   
Nice!

Just a few minor problems:

* Assert.AreEqual (expr.Method, null);

It's always better to use most appropriate method. In the cases like 
this should be
Assert.IsNull or similar.


* Assert.AreEqual(expr.Method.Name, "op_Addition");

The preferred way it's add a message or at least id to each assertion.


* BindingFlags flags = BindingFlags.Public | BindingFlags.NonPublic | 
BindingFlags.Static;

This should be constant.


* builder.Append ("[" + nodeType + "]");

I know you didn't write this code, but this is preferred when you have 
StringBuilder
to avoid double allocation.

builder.Append ("[").Append (nodeType).Append ("]");

> Also, as I check that everything is done I do some cosmetic changes to
> have the code in line with the style guidelines. Hope this is ok.
>   
Yes, no problem.

> Just tell me if the patch is fine and if does make sense to continue
> implementing System.Linq.Expressions.
>
>   
Thanks,
Marek