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

Refactor pgp_packet_body_t to be more C++. #1350

Merged
merged 1 commit into from Nov 30, 2020

Conversation

ni4
Copy link
Contributor

@ni4 ni4 commented Nov 27, 2020

This PR refactors pgp_packet_body_t and packet parsing/writing to be more C++, having cleaner code and destructor-based memory freeing.
It will be updated with the code borrowed from PR #1349 via new PRs once merged.

@ni4
Copy link
Contributor Author

ni4 commented Nov 28, 2020

There are some CI timeout issues, but those seems to be caused by GHA, investigating them.
@rrrooommmaaa Do you have any idea why windows-msvc builds are so slow (3 hours instead of 20-40 minutes)?

Copy link
Contributor

@rrrooommmaaa rrrooommmaaa left a comment

Choose a reason for hiding this comment

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

I suggest some minor corrections

/** @brief write packet header, length and body to the dst
* @param dst destination to write to.
**/
void write(pgp_dest_t &dst, bool hdr = true) noexcept;
Copy link
Contributor

Choose a reason for hiding this comment

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

hdr parameter is not documented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

* or mpi is ill-formed)
**/
bool get(pgp_mpi_t &val) noexcept;
/** @brief append chunk of the data to the packet body
Copy link
Contributor

Choose a reason for hiding this comment

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

some phantom brief here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

src/librepgp/stream-packet.cpp Outdated Show resolved Hide resolved
@ni4 ni4 force-pushed the ni4-refactor-pgp_packet_body_t branch from ae355b2 to fabf966 Compare November 29, 2020 11:02
Copy link
Contributor

@rrrooommmaaa rrrooommmaaa left a comment

Choose a reason for hiding this comment

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

LGTM now)

@ronaldtse
Copy link
Contributor

It seems that the macOS test is never completing. I've restarted it a few hours ago...?

@ni4
Copy link
Contributor Author

ni4 commented Nov 30, 2020

It seems that the macOS test is never completing. I've restarted it a few hours ago...?

Yeah, I also restarted it few times/tried on my fork. Looks like something is wrong with macOS runners, but still let's investigate it first.

@ni4
Copy link
Contributor Author

ni4 commented Nov 30, 2020

Looks like macOS 11 GHA works much faster - it finished in 20 minutes here: #1353
Let's wait and see whether 250 minutes timeout will work for 10.15.

@ni4 ni4 force-pushed the ni4-refactor-pgp_packet_body_t branch from fabf966 to 85c11e8 Compare November 30, 2020 13:13
@ni4
Copy link
Contributor Author

ni4 commented Nov 30, 2020

Merging as we have two approvals and passing CI.

@ni4 ni4 merged commit d83c688 into master Nov 30, 2020
@ni4 ni4 deleted the ni4-refactor-pgp_packet_body_t branch November 30, 2020 15:59
@ni4 ni4 added this to the v0.14.0 milestone Jan 4, 2021
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.

None yet

4 participants