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

Provide TIter &TIter::operator=(TIterator *it) signature #7633

Closed
linev opened this issue Mar 22, 2021 · 0 comments · Fixed by #7641
Closed

Provide TIter &TIter::operator=(TIterator *it) signature #7633

linev opened this issue Mar 22, 2021 · 0 comments · Fixed by #7641

Comments

@linev
Copy link
Member

linev commented Mar 22, 2021

Copy post from Mattermost:

Normally one writes:

TList *lst = new TList(); 
TIter iter(lst);

This works fine.

In many places of RooFit (and also in some other classes) one can see following syntax:

TIter iter = lst->MakeIterator();

It is also fine while where is constructor signature TIter(TIterator *it).
But if one does again:

 iter = lst->MakeIterator();

One do not get that one expects. While C++ does:

  1. creates temporary TIter instance,
  2. Calls TIter &operator=(const TIter &rhs)
  3. deletes temporary TIter instance with original TIterator object which was created by lst->MakeIterator().

Means we have unnecessary duplication of TIterator in between. Moreover, following code MAY produce wrong results:

  iter = lst->MakeIterator(kIterBackward);

It depends if assign operator implemented properly for derived TIterator classes.
Probably, we should define assign operator abstract:

virtual TIterator &operator=(const TIterator &) = 0;

To ensure that all derived classes implement it

@Axel-Naumann Axel-Naumann removed their assignment Mar 22, 2021
linev added a commit to linev/root that referenced this issue Mar 22, 2021
Let assign TIterator* directly without need of creating
temporary TIter object in between.
Solves root-project#7633
@linev linev linked a pull request Mar 22, 2021 that will close this issue
pcanal pushed a commit that referenced this issue Mar 22, 2021
Let assign TIterator* directly without need of creating
temporary TIter object in between.
Solves #7633
nicknagi pushed a commit to nicknagi/root that referenced this issue Mar 30, 2021
Let assign TIterator* directly without need of creating
temporary TIter object in between.
Solves root-project#7633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants