felixguendling / cista

Cista is a simple, high-performance, zero-copy C++ serialization & reflection library.
https://cista.rocks
MIT License
1.78k stars 113 forks source link

constexpr custom type_hash() function for custom classes #152

Closed AdelKS closed 1 year ago

AdelKS commented 1 year ago

Hello,

First of all, thanks for this awesome project !

I am opening this issue to discuss the possibility to a compile time type hashing mechanism in the library. I came up with this because I am hitting against some runtime errors when de-serializing from a faulty byte array (I am not sure what's happening, may open an issue with a reproducible minimal example later).

I saw that the needed hashing routines are constexpr already and the only thing preventing from doing everything at compile time is the std::map for complex class structures. Simple ones do not need that and can have their structure hashed at compile time.

I can think of a solution with an approach that boils down to something like

if constexpr (is_constexpr_hashable_v<T>)
  // do new compile time approach
else // do old runtime approach

where is_constexpr_hashable_v comes from a struct

template <class T> 
struct is_constexpr_hashable: std::false_type {}

to manually specialize for classese that are indeed constexpr hashable. Otherwise I can think of a more automated solution using C++20 concepts and constraints. I did not take a lot of time to think if one can stay in C++17 and do an automated approach still.

Would love your input on this.

Cheers,

Adel

felixguendling commented 1 year ago

Hey Adel, Thank you for your feedback!

I came up with this because I am hitting against some runtime errors when de-serializing from a faulty byte array

I am not sure, how having constexpr type hashing would help here? Can you please elaborate more on how exactly your problem would be solved by having constexpr type hashing?

Regarding the constexpr type hashing:

I think I could try to look into implementing a custom constexpr map for this purpose. I guess, the type lists won't be very large, so a solution with a std::array like proposed here should work.

Still, the canonicalize_type_name function is not constexpr - however, this is only needed in case the type hash should be the same across different compilers. So a simple macro switch CISTA_CANONICALIZE_TYPE_NAME switch to skip this function would suffice to be able to have a constexpr type hashing function.

But for this effort to make sense, I would need to understand how constexpr type hashing would improve the situation.

AdelKS commented 1 year ago

Hello, thank you for your feedback !

I am not sure, how having constexpr type hashing would help here? Can you please elaborate more on how exactly your problem would be solved by having constexpr type hashing?

I am getting a double free error on the map that intervenes in type_hash(), when it gets out of scope and destroyed, this function. So I thought if the hashing is taken care of not at runtime I won't hit any issues, some kind of workaround.

I only see the constexpr (and static) type hashing as a performance (no need for class instances) and quality of life improvement otherwise.

felixguendling commented 1 year ago

I think there's no way that a double-free error can happen for a simple std::map on the stack (without having a bug somewhere else). But I am happy to fix a bug should there be one in the type hashing. I think I will not implement/merge a second constexpr version of type hash to mitigate a bug (doesn't matter if it's in cista itself or anywhere else). This sounds to me like a dirty hack and I want to keep cista as clean and bug-free as possible. Please open a new issue with a minimal reproducing example.

AdelKS commented 1 year ago

I agree, I just thought of static and constexpr hashing as a linked but unrelated subject to my issue (and I think cista definitely isn't the source of it).

I opened this issue to ask about what you think about compile time and static (no instance) hashing, not about the issue I've been having. I do believe it's the opposite of a dirty addition. But it seems you gave your stance in the matter. Thanks for your time.

Adel.