MINIFICPP-2813 Upgrade thirdparty libraries part 1#2174
Conversation
lordgamez
force-pushed
the
MINIFICPP-2813_part_1
branch
from
3b944d8 to
386949e
Compare
| utils::Identifier container_id; | ||
| auto eventRead = FlowFileRecord::DeSerialize(gsl::make_span(it->value()).as_span<const std::byte>(), content_repo_, container_id); | ||
| const auto slice = it->value(); | ||
| auto eventRead = FlowFileRecord::DeSerialize(std::span<const std::byte>(reinterpret_cast<const std::byte*>(slice.data()), slice.size()), content_repo_, container_id); |
There was a problem hiding this comment.
I didn't check yet, but we should see if we can avoid the reinterpret_cast in favor of as_bytes or as_writable_bytes.
There was a problem hiding this comment.
It seems to be working, updated in 9269fbd
There was a problem hiding this comment.
There are a lot more instances of this pattern, could you go through all of them in this diff?
There was a problem hiding this comment.
I could only find one instance in the diff and one other instance in a separate test, fixed in fa0d11a Please point them out if I missed any other instances.
lordgamez
force-pushed
the
MINIFICPP-2813_part_1
branch
from
ff45cfa to
9269fbd
Compare
martinzink
left a comment
martinzink
left a comment
There was a problem hiding this comment.
One small improvement idea, but if that causes any compilation issue feel free to ignore it.
Otherwise LGTM great work 👍
| include(GslLite) | ||
|
|
||
| # Add necessary definitions based on the value of STRICT_GSL_CHECKS, see gsl-lite README for more details | ||
| list(APPEND GslDefinitions gsl_CONFIG_DEFAULTS_VERSION=1) |
There was a problem hiding this comment.
Why did you choose to delete this definition? I'd keep the config defaults version explicitly specified.
There was a problem hiding this comment.
I removed it because that's the default, but we can keep it to be explicit if you prefer.
There was a problem hiding this comment.
I think I'd prefer to keep this one explicitly specified, in order to not accidentally defeat gsl-lite backwards compatibility efforts by automatically adopting a breaking change in default settings.
There was a problem hiding this comment.
what's the source of this patch? Is it still necessary?
There was a problem hiding this comment.
I wrote it, it is needed to specify the flags manually for the bundled openssl otherwise the build tries to check these flags through compiling examples using the system installed openssl to check which feature is available.
There was a problem hiding this comment.
Can you add this info to the top of the file? (that you wrote it and because otherwise it would use the system openssl otherwise)
fyi: This patch captured my attention because it's related to openssl, and patching crypto code should be a last resort.
4 participants
This is the first part of upgrading the thirdparty libraries before releasing MiNiFi C++ 1.0:
Note: Each upgrade is done in a separate commit, so it is recommended to review the PR commit by commit for easier understanding.
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.