Copilot commented on code in PR #891:
URL: https://github.com/apache/incubator-graphar/pull/891#discussion_r2870659672
##########
cpp/src/graphar/graph_info.cc:
##########
@@ -212,15 +214,15 @@ std::shared_ptr<AdjacentList>
CreateAdjacentList(AdjListType type,
class VertexInfo::Impl {
public:
- Impl(const std::string& type, IdType chunk_size, const std::string& prefix,
+ Impl(std::string type, IdType chunk_size, std::string prefix,
const PropertyGroupVector& property_groups,
const std::vector<std::string>& labels,
std::shared_ptr<const InfoVersion> version)
- : type_(type),
+ : type_(std::move(type)),
chunk_size_(chunk_size),
property_groups_(std::move(property_groups)),
Review Comment:
property_groups is taken as a const reference, so std::move(property_groups)
will not actually move and is misleading (it will copy). Either drop std::move
here or change the parameter to be passed by value/rvalue so the move is
effective.
```suggestion
property_groups_(property_groups),
```
##########
cpp/src/graphar/arrow/chunk_reader.h:
##########
@@ -65,7 +65,7 @@ class VertexPropertyArrowChunkReader {
const std::shared_ptr<VertexInfo>& vertex_info,
const std::shared_ptr<PropertyGroup>& property_group,
const std::vector<std::string>& property_names, const std::string&
prefix,
- const util::FilterOptions& options = {});
+ util::FilterOptions options = {});
Review Comment:
FilterOptions is now passed by value in these constructors, but the various
Make(...) factory methods below still take `const util::FilterOptions&` and
pass it through, which prevents moving when callers have an rvalue options
object. To fully realize the intended move-optimization, consider updating the
Make(...) overloads to take FilterOptions by value and forward with std::move
(or use a forwarding-reference pattern).
##########
cpp/src/graphar/graph_info.cc:
##########
@@ -568,20 +570,20 @@ Status VertexInfo::Save(const std::string& path) const {
class EdgeInfo::Impl {
public:
- Impl(const std::string& src_type, const std::string& edge_type,
- const std::string& dst_type, IdType chunk_size, IdType src_chunk_size,
- IdType dst_chunk_size, bool directed, const std::string& prefix,
+ Impl(std::string src_type, std::string edge_type, std::string dst_type,
+ IdType chunk_size, IdType src_chunk_size, IdType dst_chunk_size,
+ bool directed, std::string prefix,
const AdjacentListVector& adjacent_lists,
const PropertyGroupVector& property_groups,
std::shared_ptr<const InfoVersion> version)
- : src_type_(src_type),
- edge_type_(edge_type),
- dst_type_(dst_type),
+ : src_type_(std::move(src_type)),
+ edge_type_(std::move(edge_type)),
+ dst_type_(std::move(dst_type)),
chunk_size_(chunk_size),
src_chunk_size_(src_chunk_size),
dst_chunk_size_(dst_chunk_size),
directed_(directed),
- prefix_(prefix),
+ prefix_(std::move(prefix)),
adjacent_lists_(std::move(adjacent_lists)),
property_groups_(std::move(property_groups)),
Review Comment:
adjacent_lists and property_groups are taken as const references, so
std::move(adjacent_lists) / std::move(property_groups) will not actually move
and is misleading (it will copy). Either remove std::move or accept these
parameters by value/rvalue so moves are effective.
```suggestion
adjacent_lists_(adjacent_lists),
property_groups_(property_groups),
```
##########
cpp/src/graphar/arrow/chunk_reader.h:
##########
@@ -492,7 +492,7 @@ class AdjListPropertyArrowChunkReader {
const std::shared_ptr<EdgeInfo>& edge_info,
const std::shared_ptr<PropertyGroup>& property_group,
AdjListType adj_list_type, const std::string prefix,
Review Comment:
`const std::string prefix` is passed by value and marked const, which forces
an extra copy and also prevents moving if you ever wanted to. Prefer `const
std::string& prefix` (no copy) or `std::string prefix` (and then move into
members) to keep the parameter-passing consistent and efficient.
```suggestion
AdjListType adj_list_type, const std::string& prefix,
```
##########
cpp/src/graphar/graph_info.h:
##########
@@ -75,7 +75,7 @@ class PropertyGroup {
* prefix is the concatenation of property names with '_' as separator
*/
explicit PropertyGroup(const std::vector<Property>& properties,
- FileType file_type, const std::string& prefix = "");
+ FileType file_type, std::string prefix = "");
/**
Review Comment:
PR title suggests only FilterOptions refactor, but this header change also
updates the public API for PropertyGroup/AdjacentList constructors (prefix now
passed by value). Please update the PR title/description (and note any API
compatibility implications) or split the change to keep scope clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
