Jump to content
  • Advertisement
Sign in to follow this  
Nemo Persona

Bad code or usefull ...

This topic is 1506 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

If you know other examples like this related to game libraries I would love to hear about them.

 

So I’m looking over maths libraries and most of those I have seen implement something like the struct/class below:

struct Vec3
{
  float x, y, z;

  float& Vec3::operator[](unsigned int i)
  { return (&x)[i]; }
}

Note: this has many variations, private members or array, reinterpret_cast, ... matrix representations with same structure, ... but they all result in the risk of writing or reading memory that does not belong to the class.

 

My question is why would any one think this is good and acceptable code ?

 

- It is not faster then direct access or in worse case an in-line function call to access private member.

- On top of that possible bugs resulting form the operator[] are hard to id and/or track down unless you implement bound checking but that would slow things down even more. Then we argue bounds checking is debug only, again why risk the bugs in production code in the first place ?

 

Did i miss a memo somewhere ?

Is the 'operator[]' really that useful ?

 

Just a semi rant/question ... but I would love some other opinions on this.

Edited by Nemo Persona

Share this post


Link to post
Share on other sites
Advertisement

One example I can think of is when implementing kd-trees or a few other space-partitioning trees, you usually want to rotate through each coordinate for the split axis at each level of the tree (i.e. level 1 = split on x-axis, level 2 = split on y-axis, etc..). With the [] operator it amounts to working on vec[depth % 3], without it you are stuck with branching on the depth or unrolling three tree levels at a time (awful). In short, it gives you constant-time access to any coordinate based on an index, and is the kind of thing where you are essentially screwed if it's not provided.

 

That said another, simpler answer to your question is, why not? It does not cause any performance penalty compared to direct access, can potentially help awkward situations like the one I described above, and can be made strictly safe (wrt memory layout and stuff) and standards-conforming with little effort - in fact, zero effort for a properly implemented vector class. If it is never used, it won't even be included in the binary, and it will almost certainly be inlined in all use cases being a trivial function, so all it does is take up five lines of code maximum in your vector class for only potential gains. One could then argue that features need to be proven worthwhile before being added to a library, and I won't deny that (I would like to see more examples of the use of vec[] as well) but, honestly, how many other features can you think of providing with your vector class? I can think of more questionable ones than [].

Share this post


Link to post
Share on other sites
One example I can think of is when implementing kd-trees or a few other space-partitioning trees, you usually want to rotate through each coordinate for the split axis at each level of the tree (i.e. level 1 = split on x-axis, level 2 = split on y-axis, etc..). With the [] operator it amounts to working on vec[depth % 3], without it you are stuck with branching on the depth or unrolling three tree levels at a time (awful).

Ok, branching in this case is bad, but for a specific cases like this it is still possible to cast locally and avoid the branching. By adding the operator to the class it self you create a potential problem that can be hidden any where in the code base. (where a programmer feels like typing [i + 1] in staid of getY())

 

 

That said another, simpler answer to your question is, why not?
It does not cause any performance penalty compared to direct access, can potentially help awkward situations like the one I described above, and can be made strictly safe (wrt memory layout and stuff) and standards-conforming with little effort - in fact, zero effort for a properly implemented vector class.
 
If it is never used, it won't even be included in the binary, and it will almost certainly be inlined in all use cases being a trivial function, so all it does is take up five lines of code maximum in your vector class for only potential gains.

It does not cause a performance hit but there is no performance gain ether, there is just the potential for bugs or slower and or more complex code in case you do decide to make it a secure conforming function.

 

Just relaying on the faq that no one is supposed to be using the operator "unless in specific cases" sounds bad, code stays around for a long time but programmers come and go. Just like comments or docs saying "// Do not use unless really needed" make no sense at all, "needed" is undefined and/or defined differently by different people.

 

And yes, there are some other questionable functions in a vector class but this one has the highest risk factor of all those I can come up with.

Edited by Nemo Persona

Share this post


Link to post
Share on other sites

Ok, branching in this case is bad, but for a specific cases like this it is still possible to cast locally and avoid the branching. By adding the operator to the class it self you create a potential problem that can be hidden any where in the code base. (where a programmer feels like typing [i + 1] in staid of getY())

 

.. I'm not sure I understand what you mean. You are suggesting breaking into the internals of the vector by taking a pointer to x and incrementing it in this specific case, instead of using an operator[] specially designed for the task? How does that avoid safety issues at all, especially since like I said, proper implementation of this function is strongly coupled to how the vector struct is implemented with respect to its layout and so on? Seems to me it just muddles up the codebase with random casts. The operator[] offers a specific and well-defined functionality, there is nothing vague about it. Where should the line be drawn between "this operation is used twice in the entire codebase" and "the dot product is used everywhere so we made this dot function"?

Share this post


Link to post
Share on other sites

By adding the operator to the class it self you create a potential problem that can be hidden any where in the code base.

Adding to what Bacterius already said, I'd like to add disagreement to this bit particularly. Having it cleanly in a well-defined function is good. Should there ever be a problem with the code it's enough to fix that one function. Hunting down each hand-rolled occurrence is a horrible debugging nightmare (and I'm saying this as someone who had to do it a few times in his life).
 

(where a programmer feels like typing [i + 1] in staid of getY())

You can simply add an 'assert(i < 3);' to the function. No impact on non-debug builds and a clean error signaling in debug builds.

Share this post


Link to post
Share on other sites
.. I'm not sure I understand what you mean. You are suggesting breaking into the internals of the vector by taking a pointer to x and incrementing it in this specific case, instead of using an operator[] specially designed for the task? How does that avoid safety issues at all, especially since like I said, proper implementation of this function is strongly coupled to how the vector struct is implemented with respect to its layout and so on? Seems to me it just muddles up the codebase with random casts. The operator[] offers a specific and well-defined functionality, there is nothing vague about it. Where should the line be drawn between "this operation is used twice in the entire codebase" and "the dot product is used everywhere so we made this dot function"?

That is exactly what I suggest.

 

It localizes the safety issues and minimizes overall risk. If some spatial trees are the only reason to use the [] operator the you would not really litter the entire code base with random casts, you could even implement a special private struct to make it cleaner.

 

I'm still not convinced the benefits out way the risk or that it is possible to make the operation secure without losing performance.

 

 

(where a programmer feels like typing [i + 1] in staid of getY())
You can simply add an 'assert(i < 3);' to the function. No impact on non-debug builds and a clean error signaling in debug builds.

What about non-debug builds ? Are you sure your debug process is gone catch every edge case where the operation is used ? I'm sorry but to me this sounds like raw pointers all over again.

Edited by Nemo Persona

Share this post


Link to post
Share on other sites

It localizes the safety issues and minimizes overall risk. If some spatial trees are the only reason to use the [] operator the you would not really litter the entire code base with random casts, you could even implement a special private struct to make it cleaner.

I have seen thought processes like that in code bases I had to debug. As Is said, it ended with me hunting down similar, equally broken fragments all over the place. Not an experience I wish to repeat when I have a chance to avoid it. If there is even a chance it's used in two places, it gets its own one centralized function.

Also, I can easily find more reasons why an [] operator could be convenient, for example any template code which works on vectors of { 2, 3, 4} or even arbitrary dimension and needs to access individual components.
 

(where a programmer feels like typing [i + 1] in staid of getY())

You can simply add an 'assert(i < 3);' to the function. No impact on non-debug builds and a clean error signaling in debug builds.

 

What about non-debug builds ? Are you sure your debug process is gone catch every edge case where the operation is used ? I'm sorry but to me this sounds like raw pointers all over again.

If there is any crash in release build it should be trivial to follow the same steps in debug builds to hit the assertions and quickly identify the problem. That said, a developer should spend sufficient time in debug builds so that it happening in non-debug builds is never a realistic issue.

Edit: To reinforce my point: Adding Álvaro's standard-compliant code requires me to change a single implementation. Without the single implementation I need to find the two places where it is used that I know of, then debug for half an hour to find the one spot I didn't know about and fix it thrice more over the course of several months when stumbling over the issue again after reactivating old code. Those are pretty much real-life numbers from experience, although not for this particular issue but it was again something that should have been in a single implementation. Edited by BitMaster

Share this post


Link to post
Share on other sites

The top version of the code (using (&x)[i] ), I would never ever use.

 

It's just too danagerous. It will work fine 100% of the time when you first write it, on OS a with compiler b on platform c, then you change one of them and the game crashes.

 

I've used compilers that use really strange packing systems for variables, especially when you move into 64 bits. It's entirely possible (though unlikely) to end up with the Z parameter being miles away from Y.  I actually had a bug once where someone had done something similar and looking at the memory mapping I saw .....

 

|  x   32 bits    | y 32 bits    |

| other var   64 bits           |

| other var   64 bits           |

| other 32 bits| z 32 bits    |

 

Why the hell the compiler did that ? Don't ask me! unsure.png

 

The second solution looks a lot more bullet proof, but even then I get twitchy when I see pointers to member variables.

Share this post


Link to post
Share on other sites
Personally I would probably do it for my own projects because I know what platforms it compiles on. I would however add a static assert to be immediately notified about the need for intervention, probably something like that: static_assert(sizeof(Vec3) == 3 * sizeof(float), "unexpected packing");

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!