[
https://issues.apache.org/jira/browse/MINIFICPP-515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16513998#comment-16513998
]
ASF GitHub Bot commented on MINIFICPP-515:
------------------------------------------
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/359#discussion_r195782604
--- Diff: libminifi/include/core/Property.h ---
@@ -65,15 +65,15 @@ class Property {
dependent_properties_(std::move(dependent_properties)),
exclusive_of_properties_(std::move(exclusive_of_properties)),
is_collection_(false) {
- values_.push_back(std::string(value.c_str()));
--- End diff --
I'm curious why this existed the way it did, but my guess is it's
overriding the C++11 pointer copy to take ownership of that pointer. We have a
named variable already and std::string has a move constructor so most of this
is implicitly unnecessary [1], but I'm more concerned with the impetus for the
ptr and std::string. That may have been someone ( could be me ) taking
advantage of the ptr copy. I'll do al little research into that to validate,
but in general push_back of already constructed objects with a move constructor
should be fine; however, even in the case above I would have preferred
emplacing the pointer than pushing since we're constructing an object to be
appended, so I'll take a look at the string specs before approving to see if I
can gather what the original intent was, here. My be a micro-optimization for
pointer copies...but I think the move constructor does that for for SSOs, which
many of these may be.
[1] http://ptspts.blogspot.com/2017/02/fast-vector-append.html
> Implement emplace_back for values in Property class
> ---------------------------------------------------
>
> Key: MINIFICPP-515
> URL: https://issues.apache.org/jira/browse/MINIFICPP-515
> Project: NiFi MiNiFi C++
> Issue Type: Improvement
> Reporter: Andrew Christianson
> Assignee: Andrew Christianson
> Priority: Major
>
> We're calling push_back instead of emplace_back, as well as unnecessarily
> rebuilding strings from c_string() values.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
