Skip to content

Commit

Permalink
resolve UB by using copy instead of reference
Browse files Browse the repository at this point in the history
  • Loading branch information
@krlmlr
krlmlr committed Sep 15, 2017
1 parent c873a94 commit 3e9cf81
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion inst/include/dplyr/Result/VectorSliceVisitor.h
View file
Expand Up @@ -29,7 +29,7 @@ class VectorSliceVisitor {
}

private:
const Vector<RTYPE>& data;
Vector<RTYPE> data;
int n;
const SlicingIndex& index;
};
Expand Down

3 comments on commit 3e9cf81

@hadley
Copy link
Member

@hadley hadley commented on 3e9cf81 Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a potentially costly change since we'll now be copying vectors of data? Or does Vector contain a SEXP?

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented on 3e9cf81 Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rcpp::Vector contains an SEXP (ref-counted via R_PreserveObject() and R_ReleaseObject()). No copies or duplication involved. It does fix the problem, but a more sustainable fix involves changing references to pointers in the affected constructors, which would make passing temporary objects much harder.

@hadley
Copy link
Member

@hadley hadley commented on 3e9cf81 Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, great.

Please sign in to comment.