Skip to content

Conversation

@KaganCanSit
Copy link
Contributor

@KaganCanSit KaganCanSit commented Oct 11, 2025

Hello Botan Developer Team,

This pull request handling Clang-Tidy suppression in the tmp2_session.h file.

- Mark SessionBundle constructor as explicit
- Mark SessionHandle::operator ESYS_TR() as explicit

Source: C++ Core Guidlines C.46 By default, declare single-argument constructors explicit

UPDATE:
This PR updates FIXME comments in tpm2_session.h to properly document the intentional use of implicit conversions in TPM2 session wrapper classes.

  • Updated SessionHandle::operator ESYS_TR() NOLINTNEXTLINE comment
  • Updated SessionBundle constructor NOLINTNEXTLINE comment
  • No functional changes to code behavior

If need any change or edits, please write comment to me. I fix.
Regards.

@KaganCanSit KaganCanSit marked this pull request as draft October 11, 2025 17:51
@KaganCanSit KaganCanSit force-pushed the fixme/tmp2-explicit-conversions branch 5 times, most recently from 570f06d to 3391122 Compare October 12, 2025 11:33
@coveralls
Copy link

coveralls commented Oct 12, 2025

Coverage Status

coverage: 90.67% (-0.002%) from 90.672%
when pulling a8a77af on KaganCanSit:fixme/tmp2-explicit-conversions
into b88037c on randombit:master.

@KaganCanSit KaganCanSit force-pushed the fixme/tmp2-explicit-conversions branch from 3391122 to 63a4253 Compare October 12, 2025 13:15
@KaganCanSit KaganCanSit marked this pull request as ready for review October 12, 2025 14:43
Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

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

Unfortunately I have to oppose this PR. The implicit conversion operators and constructors were intentional for API ergonomics. The required adaptions in the implementation (adding static_cast and explicit constructor calls in many places) are exactly what they were meant to avoid. 🙂

@KaganCanSit
Copy link
Contributor Author

Unfortunately I have to oppose this PR. The implicit conversion operators and constructors were intentional for API ergonomics. The required adaptions in the implementation (adding and explicit constructor calls in many places) are exactly what they were meant to avoid. 🙂static_cast

Alright. Then I'm retrieving the header contents here in a similar way to #5119.

  • Remove explicit qualifiers.
  • Update FIXME messages.
  • Review the remaining conversions. Remove or edit “static_cast” contents.

Here, along with “SessionBundle,” most likely this entire branch will be reverted and only the FIXME messages will be updated, but I'm not sure. I'll look into it.

Thanks for review.

Update NOLINTNEXTLINE comments to document that implicit converions
in SessionHandle and SessionBundle are intentional design choices
for C API wrapper ergonomics, not issues to be fixed.
@KaganCanSit KaganCanSit force-pushed the fixme/tmp2-explicit-conversions branch from 63a4253 to a8a77af Compare October 15, 2025 20:18
@KaganCanSit KaganCanSit changed the title Resolve FIXME: Enforce explicit conversions in TPM2 Session Header File Update TPM2 session FIXME comments to document intentional implicit conversions Oct 15, 2025
@KaganCanSit
Copy link
Contributor Author

Unfortunately I have to oppose this PR. The implicit conversion operators and constructors were intentional for API ergonomics. The required adaptions in the implementation (adding and explicit constructor calls in many places) are exactly what they were meant to avoid. 🙂static_cast

Alright. Then I'm retrieving the header contents here in a similar way to #5119.

  • Remove explicit qualifiers.
  • Update FIXME messages.
  • Review the remaining conversions. Remove or edit “static_cast” contents.

Here, along with “SessionBundle,” most likely this entire branch will be reverted and only the FIXME messages will be updated, but I'm not sure. I'll look into it.

Thanks for review.

Thank you for the feedback again. I apologize for the confusion.

I misunderstood the scope - I focused on the FIXME comments but incorrectly assumed the implicit conversions should be removed. I now understand they are intentional design choices for API ergonomics.

I updated this PR to only replace the FIXME comments in tpm2_session.h with proper explanations, and revert all implementation changes. And I change commit, PR text contents.

Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

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

Thanks!

@reneme reneme merged commit 3c6caa3 into randombit:master Oct 16, 2025
45 checks passed
@KaganCanSit KaganCanSit deleted the fixme/tmp2-explicit-conversions branch October 16, 2025 05:15
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.

3 participants