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

Add Conditional Type Support for Variadic Read and Write Method #2

Closed

Conversation

toprakmurat
Copy link

Summary

This pull request introduces a new feature to the binary_stream class, which conditionally checks if types support the required write and read features when using the variadic read and write methods. This enhancement ensures type safety and compatibility without adding complexity to the other read and write overloads.

Changes

  • Implemented hasSupportedFeature: A struct that conditionally checks if types support the write and read features.
  • Updated Variadic read and write Methods: Applied the hasSupportedFeature check to enforce type compatibility.
  • Maintained Simplicity: Ensured that the individual read and write functions remain straightforward and unaffected by the new type checks.

Motivation

The new feature aims to enhance type safety when using the variadic read and write methods, ensuring that only compatible types are processed. By limiting the scope of the hasSupportedFeature check to the variadic method, we avoid unnecessary complexity in the other ``readandwrite` overloads.

Please review the changes and provide feedback. Thank you!

- Implemented `hasSupportedFeature` to conditionally check if types support required `read` and `write` features.
- Applied `hasSupportedFeature` check both to the variadic `read` and `write` function to enforce type compatibility.
- Ensured other `read` and `write` overloads remain unaffected to avoid unnecessary complexity.
@farukeryilmaz
Copy link
Owner

farukeryilmaz commented Jul 22, 2024

Thank you for the effort you've put into this pull request. Here's some feedback to refine the pull request:

Build Error

The pull request currently does not pass the GitHub Actions checks. Specifically, build fails on the variadic read-write methods. This indicates that the implementation may be incorrect or incompatible with the existing codebase. It's essential to ensure that all tests pass to maintain the stability and reliability of the library.

Redundancy and Existing Concepts

The current implementation uses the NetworkSerializableType concept, covering types like fundamental types, arrays, vectors, and strings. This ensures that the types are compatible with the serialization (write) and deserialization (read) methods. The introduction of your proposed changes might seem redundant, but they do offer some advantages:

  • Explicit Checking for Method Availability: This enhancement ensures that types are not only serializable but are confirmed to have specific implementations for serialization and deserialization, which is a more explicit approach than the current type checks.
  • Scalability in Code Maintenance: As the library evolves to handle more complex types, the explicit checks can prevent potential bugs by ensuring each type fully implements necessary methods.
  • Documentation and Clarity: The new concept can serve as self-documenting code, clearly indicating the requirements for future maintainers and users.

Modern C++20 Practices:

Instead of relying on SFINAE, which can lead to more complex and harder-to-read code, I recommend leveraging C++20 concepts. This will not only simplify the syntax but also improve code readability and maintainability. Also hasSupportedFeature is not sufficiently descriptive or meaningful within the context of the library. Alternative naming suggestions: DirectlySerializable, SerializationDefined or SerializeCheck.

If the new concept adequately replaces the functionalities of NetworkSerializableType, it would be reasonable to remove the NetworkSerializableType concept from the codebase if your testing confirms that the new concept adequately covers all previous use cases. This will help in keeping the codebase clean and focused.

I tried couple of solutions but ended up getting "overlapping concepts" problems since checking if read and write methods exist for a type is too generic. I had to add more concept definitions to distinct concepts, but the solutions I found got more complex then it needs to be.

Code Formatting

It appears that the changes have not adhered to the existing code style guidelines as outlined in CONTRIBUTING.md. Please use the .clang-format file provided in the project to format your code. Most modern IDEs can integrate this file to automatically or manually apply the correct formatting.

Improvement in Pull Request Title

A more descriptive title like "add DirectlySerializable Concept to Enforce Type Safety in Variadic Read and Write Methods" would better reflect the changes made and their impact on the library.

Final Notes

Please address these points, particularly ensuring that the unit tests pass and the code adheres to our formatting standards. Once these adjustments are made, I believe the pull request will be in a strong position for approval.

Thank you for your efforts to improve our serialization library.

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.

2 participants