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

vibe.data.bson const-correctness enhancements #77

Merged
merged 1 commit into from
Jul 24, 2012

Conversation

cybevnm
Copy link
Contributor

@cybevnm cybevnm commented Jul 23, 2012

"in" storage class added to parameters of some functions. This shouldn't break any existent code.

@s-ludwig
Copy link
Member

Except for the cases Bson[], Bson[string] and Json, which are definitely better off with in, does it make any difference? I ask because they should behave like pure POD values and I want to avoid redundant keywords. But if it does indeed make a difference I think there may be a compiler bug.

C++ style ref const would also be good in some places, but unfortunately - at least up to DMD 2.059 - this disallows passing return values as parameters...

@cybevnm
Copy link
Contributor Author

cybevnm commented Jul 24, 2012

Bson[] and Bson[string] are reference types both, so functions which takes them as non-const parameter can make changes to them, which will be visible to client after return. Consider this example:

import std.stdio;

void foo( uint[] arr )
{
  arr.reverse; // client will see result of reversing
}

void bar( uint[string] dict )
{
  dict["newKey"] = 1; // client will see new element
}

void main()
{
  // array mutating
  uint[] arr = [ 1, 2, 3 ];
  writeln( arr ); // output: [1, 2, 3]
  foo( arr );
  writeln( arr ); // output [3, 2, 1]

  // dictionary mutating
  uint[string] dict = [ "oldKey": 0 ];
  writeln( dict ); // output: ["oldKey":0]
  bar( dict );
  writeln( dict ); // output: ["newKey":1, "oldKey":0]
}

Note: we don't modify individual elements of containers, but containers themselves.

Now about Json type. It is not actually POD type in sense of not aliasing common data between copies of Json objects. When we copying Json object of type Json.Type.Array, the copy will share m_array reference with original. If we'll modify copy's array contents in some way, original's users will see changes. Consider this example:

import std.stdio;
import std.traits;
import vibe.d;

void foo( Json j )
{
  // we change copy here, but client will see changes
  j[ 0 ] = Json( "d" );
  j[ 1 ] = Json( "e" );
  j[ 2 ] = Json( "f" );
}

static this()
{
  // foo mutates original object through its copy
  Json j = Json( [ Json( "a" ), Json( "b" ), Json( "c" ) ] );
  writeln( j[ 0 ], j[ 1 ], j[ 2 ] ); // output: "a""b""c"
  foo( j );
  writeln( j[ 0 ], j[ 1 ], j[ 2 ] ); // output: "d""e""f"

  // traits says that json has aliasing
  static assert( hasAliasing!( Json ) ); 
}

So in required in all those situations for supporting const correctnes on client's side. If in wouldn't be added then only way for client to keep const correctnes is to copy arrays/dictionaries, and in some way make deep copy of json.

@s-ludwig
Copy link
Member

I mean excluding those three cases.. so BsonDate, BsonObjectID etc. I think those should behave like POD types and thus not require in.

But anyways, I'll merge the changes as it definitely improves the situation for Bson[] et al. and then check later if some of the ins can be safely removed - it's just two letters more after all ;)

s-ludwig added a commit that referenced this pull request Jul 24, 2012
vibe.data.bson const-correctness enhancements
@s-ludwig s-ludwig merged commit 4fd9de5 into vibe-d:master Jul 24, 2012
@cybevnm
Copy link
Contributor Author

cybevnm commented Jul 24, 2012

Oops, I misinterpreted your comment, sorry.
I've checked PODness of other Bson-family types and all of them treated as pure value types by dmd 2.59, so in isn't strictly necessary for them.
Among std.datetime's types only SysTime requires in (which is strange because it looks like pure value type).

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

Successfully merging this pull request may close these issues.

2 participants