Skip to content

fix a const error in IndexSetIter::operator+()#94

Merged
emstoudenmire merged 1 commit intoITensor:masterfrom
xichuang:master
Apr 8, 2016
Merged

fix a const error in IndexSetIter::operator+()#94
emstoudenmire merged 1 commit intoITensor:masterfrom
xichuang:master

Conversation

@xichuang
Copy link
Copy Markdown
Contributor

@xichuang xichuang commented Apr 7, 2016

There exists an const error in the IndexSetIter::operator+(), which affect the following codes.

Index s1=Index("s1",4),s2=Index("s2",6); auto b=delta(s1,s2); auto c=b.inds().begin(); PrintData(*c); PrintData(*(c+1));

where the += operator in the diffinition of operaotr+() actually modifies the value of x.
The IndexSetIter::operator-() may also occurs this const error, but I haven't test it.

@emstoudenmire
Copy link
Copy Markdown
Contributor

Hi, so thanks for catching this. However I think the correct fix is not to remove the const from the function argument, but rather to make it no longer a reference at all (i.e. pass by value). Then the copy of x inside the function can be modified by the += operation, and the resulting IndexSetIter returned. Can you test this alternate code and confirm?

@xichuang
Copy link
Copy Markdown
Contributor Author

xichuang commented Apr 7, 2016

you mean
operator+(const IndexSetIter<T>& x, typename IndexSetIter<T>::difference_type d)
to
operator+(const IndexSetIter<T> x, typename IndexSetIter<T>::difference_type d)
?
That still doesn't work for me.

emstoudenmire added a commit to emstoudenmire/ITensor that referenced this pull request Apr 8, 2016
@emstoudenmire emstoudenmire merged commit 0b7606d into ITensor:master Apr 8, 2016
@emstoudenmire
Copy link
Copy Markdown
Contributor

Hi, so I meant that x should just be a regular value, so I meant to remove both the const and the reference modifiers. I just made those changes to your patch and merged them both in - thanks for catching this bug.

@emstoudenmire
Copy link
Copy Markdown
Contributor

Of course please let me know if it still doesn't work but I think it should now and I added a unit test based on your sample code.

@emstoudenmire
Copy link
Copy Markdown
Contributor

One other comment: be careful about making a delta tensor with two indices of different size. I may actually forbid it because it's not a common use-case; theoretically if you use it to "shrink" an index it acts like a projector whereas if you use it to "grow" an index it pads with zeros.

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