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

[record-hessian] Introduce RecordHessian. #14295

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

BLee-bot
Copy link
Contributor

@BLee-bot BLee-bot commented Nov 4, 2024

This introduces a part of RecordHessian.

ONE-DCO-1.0-Signed-off-by: Banseok Lee [email protected]

Related Issue : #13480
Draft PR: #13585

@seanshpark
Copy link
Contributor

seanshpark commented Nov 4, 2024

will continue review after #14297 #14298 lands and this PR is rebased on that.

@seanshpark seanshpark added the PR/NO MERGE Please don't merge. I'm still working on this :) label Nov 4, 2024
@BLee-bot BLee-bot force-pushed the rh-rh-only branch 2 times, most recently from 8591d2e to e416acd Compare November 11, 2024 01:38
@seanshpark
Copy link
Contributor

Your current commit title/message is

[record-hessian] This commit introduce record hessian.
This commit introduce record hessian.

which doesn't seem to express current changes.
1/ please fix the title/comment to represent current changes
2/ there are "update" changes and "add" changes. plz split them to separate PRs.

@BLee-bot
Copy link
Contributor Author

BLee-bot commented Nov 22, 2024

Your current commit title/message is

[record-hessian] This commit introduce record hessian.
This commit introduce record hessian.

which doesn't seem to express current changes. 1/ please fix the title/comment to represent current changes 2/ there are "update" changes and "add" changes. plz split them to separate PRs.

@seanshpark I tried splitting the code into smaller parts, but nncc-common does not allow me to compile if there are unused variables or unimplemented functions. So, could you review my code in this form?

@seanshpark
Copy link
Contributor

seanshpark commented Nov 24, 2024

if there are unused variables or unimplemented functions

you can solve this.

This commit introduce record hessian.

ONE-DCO-1.0-Signed-off-by: Banseok Lee <[email protected]>
Delete unused variables and lines.

ONE-DCO-1.0-Signed-off-by: Banseok Lee <[email protected]>
Apply feed back, make comment more precise.

ONE-DCO-1.0-Signed-off-by: Banseok Lee <[email protected]>
Add maybe_unused mark to avoid compile error.

ONE-DCO-1.0-Signed-off-by: Banseok Lee <[email protected]>
Fix formatting error due to comments.

ONE-DCO-1.0-Signed-off-by: Banseok Lee <[email protected]>
Fix formatting error with nnas_format.

ONE-DCO-1.0-Signed-off-by: Banseok Lee <[email protected]>
@seanshpark
Copy link
Contributor

you have useless header file and uncalled methods.
start with adding RecordHessian::initialize()

@BLee-bot
Copy link
Contributor Author

BLee-bot commented Nov 25, 2024

you have useless header file and uncalled methods. start with adding RecordHessian::initialize()

I split the code. That method will be implemented in later PR!
will update to remove useless headers at this time.

//namespace record_hessian
//{
//void RecordHessian::initialize(luci::Module *module); // To Be Implemented
// std::unique_ptr RecordHessian::profileData(const std::string &input_data_path); // To Be Implemented
//} namespace record_hessian

Remove useless headers and add comments

ONE-DCO-1.0-Signed-off-by: Banseok Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/NO MERGE Please don't merge. I'm still working on this :)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants