www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Wrong lowering for a[b][c]++

reply "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
A question was asked on the d-learn forum about why this throws a
RangeError:

	int[string][int] map;
	map["abc"][20]++;

This is understandable, since the compiler translates the second line to:

	map.opIndex("abc").opIndexUnary!"++"(20);

Since map["abc"] doesn't exist yet, opIndex throws RangeError before we
ever get to the ++.

I'd like to propose the following fix: if a given chained indexing
expression has any operator applied to its final result (either a unary
operator like ++ or --, or an assignment operator like +=), then instead
of translating previous indexes into opIndex, the compiler should map it
to a new operator overload, say opIndexCreate, which creates the
relevant entry with default value if it doesn't exist yet. That is to
say:

	map["abc"][20]++;

should be translated to:

	map.opIndexCreate("abc").opIndexUnary!"++"(20);

where opIndexCreate looks something like:

	Slot opIndexCreate(Key k) {
		Slot *s = findSlot(k);
		if (s is null) {
			s = createNewSlot(k);
		}
		return s;
	}

Similar changes should be made for expressions like a[b][c][d]=100, or
a[b][c][d]+=100.

In other words, if the tail of a chain of indexing operations maps to
opIndexAssign, opIndexUnary, or opIndexOpAssign, then all preceding
opIndex calls should be converted to opIndexCreate instead.

Comments?


T

-- 
One Word to write them all, One Access to find them, One Excel to count them
all, And thus to Windows bind them. -- Mike Champion
Mar 21 2012
next sibling parent reply "David Nadlinger" <see klickverbot.at> writes:
On Wednesday, 21 March 2012 at 18:27:30 UTC, H. S. Teoh wrote:

 A question was asked on the d-learn forum about why this throws 
 a
 RangeError:

 	int[string][int] map;
 	map["abc"][20]++;


Wait a second – aren't AAs _supposed_ to throw if accessing a 
key that doesn't exist yet? To be able to increment something, 
there already has to be a value to start from…

David
Mar 21 2012
next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Wed, Mar 21, 2012 at 07:29:44PM +0100, David Nadlinger wrote:

 On Wednesday, 21 March 2012 at 18:27:30 UTC, H. S. Teoh wrote:

A question was asked on the d-learn forum about why this throws a
RangeError:

	int[string][int] map;
	map["abc"][20]++;


 
 Wait a second – aren't AAs _supposed_ to throw if accessing a key
 that doesn't exist yet? To be able to increment something, there
 already has to be a value to start from…


[...]

If it doesn't exist, it should default to typeof(value).init, IMO.

But perhaps that was the wrong example to use. What if you wanted to do
this:

	map["abc"][20] = 1;

Currently this doesn't work. You have to explicitly create map["abc"]
first.


T

-- 
Time flies like an arrow. Fruit flies like a banana.
Mar 21 2012
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, March 21, 2012 12:00:36 H. S. Teoh wrote:

 On Wed, Mar 21, 2012 at 07:29:44PM +0100, David Nadlinger wrote:

 On Wednesday, 21 March 2012 at 18:27:30 UTC, H. S. Teoh wrote:

A question was asked on the d-learn forum about why this throws a

RangeError:
 int[string][int] map;
 map["abc"][20]++;


 
 Wait a second – aren't AAs _supposed_ to throw if accessing a key
 that doesn't exist yet? To be able to increment something, there
 already has to be a value to start from…


 
 [...]
 
 If it doesn't exist, it should default to typeof(value).init, IMO.
 
 But perhaps that was the wrong example to use. What if you wanted to do
 this:
 
 map["abc"][20] = 1;
 
 Currently this doesn't work. You have to explicitly create map["abc"]
 first.


Which I think makes perfect sense. I think that implicitly creating elements 
is evil. It's already bad enough IMHO that the language implicitly creates the 
AA itself (especially when you consider stuff like the bugs that occur when you 
pass an AA to a function expecting the elements that it adds to it to be in 
the AA afterwards - it works with an already initialized AA but not a null 
one).

I think that Steven's suggestion of a function for this makes far more sense.

- Jonathan M Davis
Mar 21 2012
prev sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Wed, Mar 21, 2012 at 05:21:22PM -0400, Jonathan M Davis wrote:

 On Wednesday, March 21, 2012 12:00:36 H. S. Teoh wrote:


[...]

 But perhaps that was the wrong example to use. What if you wanted to
 do this:
 
 map["abc"][20] = 1;
 
 Currently this doesn't work. You have to explicitly create
 map["abc"] first.


 
 Which I think makes perfect sense. I think that implicitly creating
 elements is evil.


[...]

I disagree. If you wrote:

	if (map["abc"][2] == 1)

then I agree that this should throw a RangeError.

But in the case of writing map["abc"][20]=1, you're explicitly saying
"assign 1 to map["abc"][20]". This means the user *wants* to create a
new entry in the AA. So it should create the intermediate entry
necessary as well.

Otherwise, by your reasoning, this should fail too:

	int[string] aa;
	aa["abc"] = 1;	// should throw RangeError since entry doesn't exist

which I think reduces the usability of the indexing syntax.

Having the opIndexAssign syntax work only when there's a single level of
indexing seems to me completely arbitrary.

Having said that, though, I'm a bit on the fence about making ++ or --
work in this case too. Those cases may be arguable because you need
something to exist before you can operate on it, so it may be OK to
throw in those cases. However, in the straight assignment case, I
definitely contend that it needs to work.


T

-- 
BREAKFAST.COM halted...Cereal Port Not Responding. -- YHL
Mar 21 2012
prev sibling next sibling parent reply =?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:
On 03/21/2012 11:29 AM, H. S. Teoh wrote:

 A question was asked on the d-learn forum about why this throws a
 RangeError:

 	int[string][int] map;
 	map["abc"][20]++;


Hey! That syntax is following the broken syntax of C and C++. ;)

This works:

import std.stdio;

void main()
{
     int[string][int] map;
     map[20]["abc"]++;

     writeln(map);
}

The output:

[20:["abc":1]]

Ali
Mar 21 2012
next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 3/21/12, Ali =C7ehreli <acehreli yahoo.com> wrote:

 This works:


For some reason thought it didn't work without double-checking. Heh. :p
Mar 21 2012
prev sibling parent "Jesse Phillips" <Jessekphillips+D gmail.com> writes:
H. S. is making a Library replacement AA and is what his example 
is concerned with.

You are correct, it seems his example has it out of order, the 
example on Learn was

int[100][string] map;
...

On Wednesday, 21 March 2012 at 18:58:34 UTC, Ali Çehreli wrote:

 This works:

 import std.stdio;

 void main()
 {
     int[string][int] map;
     map[20]["abc"]++;

     writeln(map);
 }

 The output:

 [20:["abc":1]]

 Ali
Mar 21 2012
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Wed, 21 Mar 2012 14:29:14 -0400, H. S. Teoh <hsteoh quickfur.ath.cx>  
wrote:


 A question was asked on the d-learn forum about why this throws a
 RangeError:

 	int[string][int] map;
 	map["abc"][20]++;

 This is understandable, since the compiler translates the second line to:

 	map.opIndex("abc").opIndexUnary!"++"(20);

 Since map["abc"] doesn't exist yet, opIndex throws RangeError before we
 ever get to the ++.


What about a create accessor of the map which is the same as accessing the  
map, except it ensures T.init is used at each level?

That is:

map.create["abc"][20]++;

Whether you want default-created elements or not is an important design  
decision that should not be up to the container.

-Steve
Mar 21 2012
prev sibling parent Alvaro <alvaroDotSegura gmail.com> writes:
El 21/03/2012 19:29, H. S. Teoh escribió:

 A question was asked on the d-learn forum about why this throws a
 RangeError:

 	int[string][int] map;
 	map["abc"][20]++;


IIRC, that worked fine for me a few weeks ago. And I found it to be just 
great that it did. I'll have to re-check but I remember it like this.

I had to produce a new tabular dataset from an original by accumulating 
elements (rows) sharing some attribute values.

The idea was to read that line by line, split into attributes and do:

  // this will hold the counts:

  int[int][int][int][string] count;

  // for each row, split field1 .. field5 (field4 is a string)

  count[field1][field2][field3][field4] += field5;


And that just worked IIRC, without initializing anything!

At the end, to iterate all the hyper map and write it out, we used 
nested foreachs, something like:

foreach(field1, item1; count)
     foreach(field2, item2; item1)
         foreach(field3, item3; item2)
             foreach(field4, field5; item3)
                 writeln(field1, field2, field3, field4, field5);

I'm trying to remember if we had to do something special to make it work...
Mar 21 2012