I have not much experience in D programming yet, and one of things which I frequently mess up is usage of various attributes, especially immutable and related.

Could anybody make a review my code at https://github.com/roman-d-boiko/functional-data-structures/blob/master/fds/persistent.d, https://github.com/roman-d-boiko/dct/blob/master/fe/syntax.d, and https://github.com/roman-d-boiko/dct/blob/master/fe/_/syntax.d with respect to usage of attributes (immutable, const, pure, etc.), contracts, and any other issues.

I'm redesigning my previous coding attempts to build D compiler front end, and would like to pay attention to these aspects. Please note that adding DDoc comments has been deferred because I need to learn how to use them.

On 2012-06-14 11:19, Roman D. Boiko wrote:
> I have not much experience in D programming yet, and one of things which
> I frequently mess up is usage of various attributes, especially
> immutable and related.
>
> Could anybody make a review my code at
> https://github.com/roman-d-boiko/functional-data-structures/blob/master/fds/persistent.d,
> https://github.com/roman-d-boiko/dct/blob/master/fe/syntax.d, and
> https://github.com/roman-d-boiko/dct/blob/master/fe/_/syntax.d with
> respect to usage of attributes (immutable, const, pure, etc.),
> contracts, and any other issues.
>
> I'm redesigning my previous coding attempts to build D compiler front
> end, and would like to pay attention to these aspects. Please note that
> adding DDoc comments has been deferred because I need to learn how to
> use them.

Just a note on the style. The convention is to but all attributes on front of the return type, except for "const", when "const" is referring to that the method is const and not the return type.

Another note.

class _Module : Module {...}

The underscore is unnecessary if you use fully quailed names:

class Module : fe.syntax.Module {...}

You can also use renamed imports or aliases.

-- 
/Jacob Carlborg

On Thursday, 14 June 2012 at 09:51:45 UTC, Jacob Carlborg wrote:
> Just a note on the style. The convention is to but all attributes on front of the return type, except for "const", when "const" is referring to that the method is const and not the return type.
OK

> Another note.
>
> class _Module : Module {...}
>
> The underscore is unnecessary if you use fully quailed names:
>
> class Module : fe.syntax.Module {...}
>
> You can also use renamed imports or aliases.
This is the naming convention I introduced for classes and functions which should not be referred from client code (factory methods will be used for creation). I have also put them into a separate folder, named _.

By the way, the whole data model is designed to be immutable. Should I replace const with immutable then?

On Thursday, June 14, 2012 11:51:44 Jacob Carlborg wrote:
> On 2012-06-14 11:19, Roman D. Boiko wrote:
> > I have not much experience in D programming yet, and one of things which I frequently mess up is usage of various attributes, especially immutable and related.
> > 
> > Could anybody make a review my code at
> > https://github.com/roman-d-boiko/functional-data-structures/blob/master/fd
> > s/persistent.d,
> > https://github.com/roman-d-boiko/dct/blob/master/fe/syntax.d, and
> > https://github.com/roman-d-boiko/dct/blob/master/fe/_/syntax.d with
> > respect to usage of attributes (immutable, const, pure, etc.),
> > contracts, and any other issues.
> > 
> > I'm redesigning my previous coding attempts to build D compiler front end, and would like to pay attention to these aspects. Please note that adding DDoc comments has been deferred because I need to learn how to use them.
> 
> Just a note on the style. The convention is to but all attributes on front of the return type, except for "const", when "const" is referring to that the method is const and not the return type.

That varies considerably from person to person. I don't think that there's really any agreed upon order other than the fact that most people agree that const should go on the right when it's modifying the function. Personally, I put almost everything on the right-hand side (the only exceptions being static, @property, and the access modifiers). It is true, however, that a number of people put the attributes before the return type. I really wish that they wouldn't though.

- Jonathan M Davis

On Thursday, 14 June 2012 at 09:56:08 UTC, Roman D. Boiko wrote:
> By the way, the whole data model is designed to be immutable. Should I replace const with immutable then?
The other thing I don't like is having to cast everywhere:

struct Maybe(T)
{
  immutable T* payload; // pointer to data
  pure nothrow this(T* payload)
  {
    this.payload = cast(immutable) payload;
  }
  pure nothrow @property T front() const in{ assert(!empty); }
  body{ return cast(T) *payload; }
  //...
}

OTOH, I'm not sure making input parameter immutable would be better for the user, same for function return type.

On Thursday, June 14, 2012 12:31:16 Roman D. Boiko wrote:
> On Thursday, 14 June 2012 at 09:56:08 UTC, Roman D. Boiko wrote:
> > By the way, the whole data model is designed to be immutable. Should I replace const with immutable then?
> 
> The other thing I don't like is having to cast everywhere:
> 
> struct Maybe(T)
> {
>    immutable T* payload; // pointer to data
>    pure nothrow this(T* payload)
>    {
>      this.payload = cast(immutable) payload;
>    }
>    pure nothrow @property T front() const in{ assert(!empty); }
>    body{ return cast(T) *payload; }
>    //...
> }
> 
> OTOH, I'm not sure making input parameter immutable would be better for the user, same for function return type.

I haven't had the chance to look over your code yet, but this looks _seriously_ suspect. Casting to and from immutable or cast away const are things that should be done _very_ rarely. One of the few times that casting to immutable makes any sense is when you need an object to be mutable while you put it together and then immutable afterwards. But when you do that, that reference or pointer is the _only_ reference to that data. e.g.

auto func(size_t numElems)
{
    auto arr = new int[](numElems);

    foreach(ref e; arr)
      //do something to initialize e

    return cast(immutable int[])arr;
}

Taking a function's parameter (which came from who-knows-where) and casting it to immutable is almost certainly a terrible thing to do. If there are _any_ other references to that data, you will have bugs. immutable is supposed to guarantee that the data is _never_ altered. So, when you cast something to immutable, you're telling the compiler that it's okay to treat that data as immutable and that you will _not_ have any mutable references to that data or alter it in any way. If you do, you'll be breaking the compiler's guarantees.

Ideally, you would always construct immutable objects as immutable rather than creating them as mutable and then casting them to immutable, but that's not always possible.

And taking your payload, which is immutable, and casting away immutable when you return it from front is truly bad. Casting away const or immutable and then mutating that object is _undefined_ behavior. Do _not_ do it unless you have to and know what you're doing.

These stackoverflow questions may help you understand better:

http://stackoverflow.com/questions/4219600/logical-const-in-d http://stackoverflow.com/questions/10364837/why-do-i-have-to-cast-this

- Jonathan M Davis

On Thursday, 14 June 2012 at 11:11:57 UTC, Jonathan M Davis wrote:
> I haven't had the chance to look over your code yet, but this looks
> _seriously_ suspect. Casting to and from immutable or cast away const are
> things that should be done _very_ rarely. One of the few times that casting to
> immutable makes any sense is when you need an object to be mutable while you
> put it together and then immutable afterwards. But when you do that, that
> reference or pointer is the _only_ reference to that data. e.g.
>
> auto func(size_t numElems)
> {
>     auto arr = new int[](numElems);
>
>     foreach(ref e; arr)
>       //do something to initialize e
>
>     return cast(immutable int[])arr;
> }
>
> Taking a function's parameter (which came from who-knows-where) and casting it
> to immutable is almost certainly a terrible thing to do. If there are _any_
> other references to that data, you will have bugs. immutable is supposed to
> guarantee that the data is _never_ altered. So, when you cast something to
> immutable, you're telling the compiler that it's okay to treat that data as
> immutable and that you will _not_ have any mutable references to that data or
> alter it in any way. If you do, you'll be breaking the compiler's guarantees.
>
> Ideally, you would always construct immutable objects as immutable rather than
> creating them as mutable and then casting them to immutable, but that's not
> always possible.
>
> And taking your payload, which is immutable, and casting away immutable when
> you return it from front is truly bad. Casting away const or immutable and
> then mutating that object is _undefined_ behavior. Do _not_ do it unless you
> have to and know what you're doing.
>
> These stackoverflow questions may help you understand better:
>
> http://stackoverflow.com/questions/4219600/logical-const-in-d
> http://stackoverflow.com/questions/10364837/why-do-i-have-to-cast-this
>
> - Jonathan M Davis
OK, clear. Thanks!

I was thinking that casting is bad here, but wanted to give the user ability to pass any object inside constructor. Now I see that this is a bad idea given transitivity of immutable.

In .NET readonly applies only to fields inside objects preventing assignments to them from other places than constructors. But I can assign another instance of an object with readonly fields to the same variable. Then I can pass that instance to some constructor and assign it to a readonly field.

How to achieve the same in D? As far as I understand, it is not possible for structs with immutable fields, and it is not clear for me about classes.

In other words:

Does a variable always point to the same place in memory and assigning to it simply copies data to that place? I don't understand whether it is possible to make it point to another place. That would be a mutable variable pointing to a type with immutable fields. After that I want to assign an instance of that type to an immutable field in constructor. Can I do so, or that would be bad?

On 2012-06-14 11:56, Roman D. Boiko wrote:

> This is the naming convention I introduced for classes and functions
> which should not be referred from client code (factory methods will be
> used for creation). I have also put them into a separate folder, named _.

Fair enough.

> By the way, the whole data model is designed to be immutable. Should I
> replace const with immutable then?

Probably.

-- 
/Jacob Carlborg

On 2012-06-14 14:47, Roman D. Boiko wrote:

> I was thinking that casting is bad here, but wanted to give the user
> ability to pass any object inside constructor. Now I see that this is a
> bad idea given transitivity of immutable.

If you want that and have it immutable you need to make a deep copy of the passed in object to be safe. You could use const instead and also making the argument const. If an argument is const you can pass both mutable and immutable values, and const of course.

> In .NET readonly applies only to fields inside objects preventing
> assignments to them from other places than constructors. But I can
> assign another instance of an object with readonly fields to the same
> variable. Then I can pass that instance to some constructor and assign
> it to a readonly field.

You should be able to use const.

> How to achieve the same in D? As far as I understand, it is not possible
> for structs with immutable fields, and it is not clear for me about
> classes.
>
> In other words:
>
> Does a variable always point to the same place in memory and assigning
> to it simply copies data to that place? I don't understand whether it is
> possible to make it point to another place. That would be a mutable
> variable pointing to a type with immutable fields.

That's only possible with pointers in D.

> After that I want to
> assign an instance of that type to an immutable field in constructor.
> Can I do so, or that would be bad?

-- 
/Jacob Carlborg

On Thursday, 14 June 2012 at 12:47:34 UTC, Roman D. Boiko wrote:
> On Thursday, 14 June 2012 at 11:11:57 UTC, Jonathan M Davis wrote:
[...]
>> - Jonathan M Davis
> OK, clear. Thanks!
[...]

I've tried to convert return types of all functions to immutable to prevent the need for casting: https://github.com/roman-d-boiko/functional-data-structures/commit/98c317b59b329fe06ffae4fc4c4ab338541e3321

I would be more happy to have them mutable with immutable fields. But since I return data stored in the immutable field, I had to cast away immutable before returning, and cast to immutable in constructor, so I gave up.

But now, with everything immutable, I had to comment out several test cases. I cannot pass an immutable struct allocated on stack, into a method by reference, and then store a pointer to it, because compiler says it is not an l-value. Should I allocate it on heap? Or get rid of functions taking parameter by ref, and use only those which take a pointer?