search

kcat / alure

Alure is a utility library for OpenAL, providing a C++ API and managing common tasks that include file loading, caching, and streaming
zlib License
70 stars 20 forks source link

Why did the implementation of alure::AttributePair changed from std::pair to struct? #33

Closed McSinyx closed 4 years ago

McSinyx commented 4 years ago

I am trying to wrap alure in Cython and the implementation of alure::AttributePair caused a bit of inconvenience so I decided to investigate further. To my surprise, it used to be implemented as std::pair instead of a custom struct, but 82436dd3eb834635140a9e8d3224376ae2d6fbf5 changed it to the current one. Another thing I cannot wrap my mind around is that why the attributes has to be null-termination (i.e. {0, 0}).

Would it be more logical to revert back to the previous implementation, or preferably, use std::unordered_map? It would not be a problem for me if it stays as is, but I'm desperate for an explanation for 82436dd3eb834635140a9e8d3224376ae2d6fbf5.

Cazadorro commented 4 years ago

I'm unsure of the exact reasoning, but I can tell you for me, std::pair is kind of annoying to deal with, .first and .second don't really specify what the members mean, plus AttributePair is self documenting in a way.

&std::get<0>(attrs.front()) is way less readable than &attrs.front().mAttribute in my opinion. I don't really have an easy place to figure out what conceptual member I'm std::geting. mAttribute makes this easy, I'm getting a reference to the attribute part of the attribute pair array. It also has the added bonus of auto-completing in the IDE so I technically have less to type (not that this is a reason to change it).

This is a common pattern outside of this repo as well, and even in other languages.

kcat commented 4 years ago

The AttributePair struct, besides giving more descriptive names to the members, helps guarantee the attribute and value are stored in a trivially copyable type, essentially meaning the raw data can be accessed (std::pair is rather complicated when it comes to being trivial). The reason for this is the same reason the attributes need null termination, because it's passed directly to OpenAL without further interpretation. OpenAL itself requires either a null attribute list, or a contiguous sequence of integers representing a series of {attribute, value} pairs that ends with a 0 attribute. If the array given to Alure doesn't have the null termination, it needs to be copied to ensure there is one.

The reason std::unordered_map can't be used is because of that contiguous layout requirement. Also, despite its name, an unordered_map does have ordering, just that the app can't rely on any particular order (it uses hashed keys, where the hashing algorithm is implementation-defined).

McSinyx commented 4 years ago

Thank you both for the prompt explanation! My mind now settled so I can continue on wrapping this in Cython. I guess I can close the issue at this moment.