-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Documentation of properties could be improved #8
Comments
As stated in the README, this is a standalone version of the meta subdir from Probably the best bet is to use the model of |
Of course, using strcmp is not the fastest approach but at least it's guaranteed to be correct and reliable, while using a hash always exposes you to the risk of a hash collision. If the user wants to use hashes for best performance, he/she could just declare a custom literal operator "_hash" and write "hello"_hash instead of just "hello". OTOH, if you used hash internally, the user would have no way to get back to strcmp, if desired. The hash approach is is correct in |
I'd use integers for the identifiers to be honest. They are pretty handy: you can use hashed strings ofc, but also runtime or compile-time generated ids as well as identifiers obtained through a pre-processing step. Moreover, they are definitely by far faster than comparing strings. That said, the idea of setting the name of the type from its id hasn't revealed a good choice. So, to sum up, rather than updating the doc to reflect the current behavior, I'd change the latter to make it closer to that of |
I agree that integers are better for the task of identifying properties, but I believe enumerators are even better. They behave like integers but they automatically also acts as a sort of "namespace", as enumerators from different enumerations do not clash even if they have the same numerical value. If I'm not mistaken you use enums in the test suite, which is good. I have to trust you if you say the EnTT approach is better and I see no problem if you decide to go that way. The second best alternative remains to just rewrite the docs to use enums instead of strings. |
The only problem with enums here is that we don't know in advance what type the user is going to use. On the other side, Does it make sense? |
I lost you. Allow me to rephrase. I was referring to the following idiom, similar to the one that
I could have used |
Oh, sorry, I thought we were speaking of the names for the meta types, like:
Instead of:
Enums already work like a charm with properties afaik. 👍 So, to sum up, the idea is to use integers (hashed strings or whatever, it's up to the user to decide it) for meta types' names, as it happens in |
Yes, they do!
That's a good plan. I like it. |
I've pushed the version that uses numeric identifiers on the branch --- EDIT I've updated also the README file. If you don't spot errors, I'll merge everything on |
I checked the wip branch and it seems good to me. |
There are a couple of issues in the documentation of properties. First, the following example is provided:
In this case, the first element of the pair, which is used to identify the property, will decay to a
const char*
and then stored in ameta::any
object. Subsequently, properties are found applying operator==
onmeta::any
instances, which is implemented by comparing the value of the pointers. This is a problem, because string literals potentially all have unique addresses, so the property cannot be portably and reliably be retrieved at later time.The retrieval example is also problematic:
since this doesn't compile on at least one compiler (Visual Studio 2019). The problem is that in this case the literal string does not decay to a pointer and apparently the
meta::any
constructor is not able to accept aconst char (&)[8]
.These two issues could be solved by using
std::string
as the type of the property key. However, the use case described in the documentation seems legit and useful, so if that's the intent, we should fix the code instead so that it works as described. There may be multiple ways to achieve this, for example, we could add the following non-template overload of themeta::type::prop
method:The text was updated successfully, but these errors were encountered: