Skip to content

refactor(C++): change FilterOptions parameter to be moved instead of const reference#891

Open
SYaoJun wants to merge 3 commits intoapache:mainfrom
SYaoJun:0301_move
Open

refactor(C++): change FilterOptions parameter to be moved instead of const reference#891
SYaoJun wants to merge 3 commits intoapache:mainfrom
SYaoJun:0301_move

Conversation

@SYaoJun
Copy link
Contributor

@SYaoJun SYaoJun commented Mar 1, 2026

Reason for this PR

use std::move instead of const refernce

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

…const reference

Signed-off-by: syaojun <libevent@yeah.net>
@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.60%. Comparing base (d535a43) to head (8007286).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #891      +/-   ##
============================================
+ Coverage     79.81%   80.60%   +0.78%     
  Complexity      615      615              
============================================
  Files            93       94       +1     
  Lines         10296    10709     +413     
  Branches       1055     1055              
============================================
+ Hits           8218     8632     +414     
+ Misses         1838     1837       -1     
  Partials        240      240              
Flag Coverage Δ
cpp 70.90% <100.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@yangxk1 yangxk1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors several C++ constructors to take parameters by value and std::move them into members, aiming to reduce unnecessary copies (notably for util::FilterOptions and some std::string prefixes).

Changes:

  • Update PropertyGroup / AdjacentList constructors to take prefix by value and move into members.
  • Update VertexPropertyArrowChunkReader / AdjListPropertyArrowChunkReader constructors to take util::FilterOptions by value and move into members.
  • Update internal VertexInfo::Impl / EdgeInfo::Impl constructors to take some std::string fields by value and move into members.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
cpp/src/graphar/graph_info.h Public API ctor signatures updated to pass prefix by value.
cpp/src/graphar/graph_info.cc Moves prefix/type strings into members; internal ctor signatures adjusted.
cpp/src/graphar/arrow/chunk_reader.h Public ctor signatures updated to pass FilterOptions by value; one ctor still takes prefix by-value-const.
cpp/src/graphar/arrow/chunk_reader.cc Implementations updated to move FilterOptions into members.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

SYaoJun and others added 2 commits March 2, 2026 21:23
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Jason Yao <libevent@yeah.net>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Jason Yao <libevent@yeah.net>
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.

4 participants