Skip to main content
GameDev.net gamedev.net
๐Ÿ”’ Locked

Is this proper usage of non-member functions?

Started by ShapeSpheres Jan 17, 2024 at 4:58 AM 19 replies 11.2k views
Original Post
ShapeSpheres
ShapeSpheres

I recently learned how I can simplify/improve code by using structs and non-member functions so to test this new discovery I refactored my camera code and I have a feeling I misused it.

I took every member function that wasn't 100% necessary out and I'm left with:

struct Camera {
	glm::vec3 position;
    glm::vec3 forward;
    glm::vec3 right;
    glm::vec3 up;

    float yaw;
    float pitch;
    float roll;

    float fov;
    float aspectRatio;
    float nearPlane;
    float farPlane;

    std::unique_ptr<CameraBehavior> behavior;

    void Update(float timestep, Input& input) {
        if (behavior) {
            behavior->Update(*this, timestep, input);
        }
    }
};

and made those member functions into non-member functions:

void Move(Camera& camera, const glm::vec3& direction, float speed, float timestep);
void Pivot(Camera& camera, float mouseXChange, float mouseYChange, float sensitivity);
void UpdateDirectionVectors(Camera& camera);

glm::mat4 ComputeViewMatrix(const Camera& camera);
glm::mat4 ComputeProjectionMatrix(const Camera& camera);

The first 3 functions probably won't ever be used by something that isn't a camera so I have a feeling those might be bad usages. The compute functions on the other hand the camera doesn't really need that it's just for the render and potentially other code so is this a better usage?

Alberth
Alberth

Your test is working fine I think ๐Ÿ™‚

You can indeed argue that the first three functions mostly โ€œbelongโ€ to a camera. That notion is indeed much less present with the 4t and 5th function. Whether making them independent functions is โ€œgoodโ€ or โ€œbadโ€ (for each case) is a decision you are learning about now. That notion however is not universal, different persons have different ideas about what โ€œgoodโ€ / โ€œ badโ€ is.

In general, independent functions have varying degrees of โ€œbelongingโ€ to something. Somewhere in the range from โ€œcompletely belong to one of the parametersโ€ to โ€œbelongs to none of them in particularโ€ is the cutting point between member functions and independent functions. That point is different for everyone, your journey is now about finding that point for yourself.

My advice is go with the flow. Keep member functions when you think there is a strong relation, like you expressed in the question, and independent functions when you think that relation is not clear enough to make it a member function. You will probably find that decisions here will shift as you get more experience, and that's ok. It takes time to find your balance.

Juliean
Juliean

I've personally come to prefer member-functions, where possible.

camera.ComputeViewMatrix();

is to me preferable to

ComputeViewMatrix(camera);

syntax-wise, but that might be just personal preference. So I would not have split those functions you showed myself. I just don't think you get miuch value from splitting functions that then always take a โ€œCamera" object as first parameter, into independant functions. Well, the one real benefit of free functions is that they might allow you to forward-declare the function, without having to include the header (which you cannot do with member-functions; and can save some inclusion-dependencies especially when used in another header).

Other than that, kind of agree with Alberth. If a bunch of function use โ€œcameraโ€ but are not really an intricate functionality of the camera object, it could make sense to separate. I'd try to think along the terms of โ€œIf I removed those functions entirely, would Camera still be usableโ€? I think in terms of not being able to move, or compute matrices, then answer would be โ€œnoโ€, and I would further be discouraged from decoupling them.

I'd probably rather go for trying to extract generic algorithms. Make a โ€œcomputeViewMatrxโ€ that takes the member-variable of the camera as parameters, and use that internally in Camera::ComputeViewMatrix, if you care about reusability.

I'm pretty sure there will be someone making a case for having free functions about members, so millega might vary. Just pick what you think makes sense, try it out, and if you don't like it, change it.

frob
frob

Understanding WHY you are doing things is important, and it looks like you're well on your way for that.

Some questions:

Why are you treating a camera object different from other game objects? There are some possible reasons for it, but most major game engines treat cameras as an add-on (a behavior or component) that you can attach to a world object; most of the manipulation is done as a general object rather than as a camera. Moving things, following, pointing at, they're not unique to cameras as you pointed out, and they can be useful on many types of objects.

Do you have reasons for separating out behavior the way you did? Are there ways to have additional cameras? Some examples include VR-viewpoint cameras that often have two stereo cameras for the eyes plus a third camera for the screen, or for cinematic cameras with special effects, or for software-cameras like minimaps or picture-in-picture displays as the camera feeds to a texture.

There's no right or wrong answer, but there are tradeoffs for each implementation choice that can help with understanding why professional tools make the choices they did, and reasons people sometimes will customize away from those choices.

JoeJ
JoeJ

DSICLAIMER: Do not read this section. It's a bad proposal. ; )

ShapeSpheres said:
The compute functions on the other hand the camera doesn't really need that it's just for the render and potentially other code so is this a better usage?

Personally i came up with an unusual solution here. But there is a catch, so i'm open for proposals if anyone knows.

Instead camera i use a mesh for example:

namespace MeshProcessing
{
	class HEMesh
	{
	public:


// some data...

		
		typedef int32_t Index;
		
		// vertices
		std::vector<Index> mVertexEdge;
		std::vector<vec> mVertexPosition;
		
		
// ...

// some functions


		void SetVertexPos (const int vertex, const vec &pos)	{ mVertexPosition[(Index)vertex] = pos; }
		vec GetVertexPos (const int vertex)		const { return mVertexPosition[(Index)vertex]; }
		
		// ...

The functions are general stuff i need everywhere. Accessing the data, but also calculating a vertex normal or polygon area, basic things.

I could not add all specific functionality to the same mesh class. It would get too big.
I need mesh editing functions, visualization, serialization, conversation, and even more specific stuff like Delaunay triangulation.

So i use derived classes for each of those topics, for example:

namespace MeshProcessing
{
	class HEMesh_Modeling : public HEMesh
	{
	public:

		int AddPolygon (const int edge,
			const int atIndex = -1);

		int AddPolygonFromEdges (const int *edgeIndices, const int numEdges,
			const int atIndex = -1);

		int AddPolygonFromVertices (const int *vertexIndices, const int numVertices,
			const int atIndex = -1);

		void RemovePolygon (const int poly);
		
		// ...

So i have all my editing functions grouped on one derived class to keep things organized by topic.

To use the derive class, i do a cast:

HEMesh mesh;
((HEMesh_Serialization&) mesh).LoadFromFile("...");
((HEMesh_Modeling&) mesh).AddPolygon(17);
((HEMesh_Vis&) mesh).RenderPolygonsFilled();

Maybe this looks a bit awkward, but it's better than giving a mesh as parameter to all those specific functions while avoiding the derived class.
It's actually nice for things which have a lot of functionality associated to it. But surely overkill to a Camera.

But the catch is: The derived classes must not introduce any new member data to the base class. Otherwise i could not cast one to the other just to โ€˜switch topicsโ€™.
And sadly C++ has no way to enforce a derived class can only add functions but no data, afaict.
If it had, my proposal here would make much more sense.

Alternatively, we could dump all functionality to the header file of mesh class definition, but use multiple C++ files to group by multiple topics. But then our header file becomes bloated with lots of inlined functions we need only rarely, which is bad for many reasons.

So i really wonder what other programmers do here. There should be a better, standard way to do group class functionality, but i could not find any. And actually i feel like misusing the concept of derived classes for a different purpose.

ShapeSpheres said:
I recently learned how I can simplify/improve code by using structs and non-member functions so to test this new discovery I refactored my camera code and I have a feeling I misused it.

Ha, it's actually coincidence i've used the word โ€˜misuseโ€™ as well. : )

But coming from C, to me it's member functions which was the new discovery.
Still, today i do not use global functions anymore. I use namespaces to group them by some topic, so they are no longer global but technically still the same.
Maybe you can do the same to make non-member functions feel less alien.

Example:

namespace TextureFilter
{
	// box filter

	template <typename T>
	static T SampleTextureBox (const T *texture,
		const int *gridI0, const float *gridF0, 
		const int *gridI1, const float *gridF1, 
		const int *size);
	
	template <typename T>
	static T SampleTiledTextureBox (const T *texture,
		const int *gridI0, const float *gridF0, 
		const int *gridI1, const float *gridF1, 
		const int *size,
		const bool tileH, const bool tileV);
		
	// ...
};

Those functions give me similar functionality then a texture lookup on GPU. It has lots of functions, so it's convenient to group them all by a namespace.

I could use a struct or class without any member data for the same purpose, but by using a namespace it's obviously clear there is no member data right from the start to anybody. So the namespace is clearly better.

However, sometimes it is useful to have private functions internal to the topic as well, to reduce visible complexity to the outside.
Then i use a class instead a namespace. Again i would like to make clear the class has no member data.

Following this, you could end up with something like this:

Camera cam;
cam.Update(...);
auto m = CameraTools::ComputeViewMatrix(cam); // CameraTools would be a namespace / class / struct to group related functionality

But as others said, this as a matter of finding your personal preferences with time, or the ability to adopt conventions from the code base you are working on with others.

Juliean
Juliean

JoeJ said:
Maybe this looks a bit awkward, but it's better than giving a mesh as parameter to all those specific functions while avoiding the derived class.

God, sorry to say, but this is horrendeous. This is in now way better than passing a mesh to the parameters.

You are relying on c-style casts here, which not only don't enforce the condition with data-members you mentioned, butr can literally cast anything. Not only will it cast const away quitely, but you could pass it any type of mismatching object, including primitive types like โ€œintโ€, and it would quietly accept it, try to call the function on that mismatching object and crash. Please, for the love of god, never recommend this to anyone. I'm not one to judge preferenced, but this is plainly bad. At least use a static_cast:

static_cast(mesh).LoadFromFile("...");

That will enforce const, and that โ€œmeshโ€ actually is a base-object of HEMesh_Serialization. However, in reality, if you really want to avoid passing a paramter, because you have like 50 different functions that all would take โ€œmeshโ€ as the first param, this is a much better solution:

struct HEMesh_Modeling

{

HEMesh_Modeling(HEMesh& mesh) : m_mesh(mesh) {}

// use m_mesh internally

int AddPolygon (const int edge,

const int atIndex = -1);

private:

HEMesh& m_mesh;

}

HEMesh_Modeling(mesh).AddPolygon();

Can't wrap it in code-tags, sorry. But this uses the same amount of code on the using-side (actually less syntax), requires no passing of parameters to the functions, and doesn't rely on any rigid fake inheritance-structure.

EDIT: Didn't even mention that what you do is UB, and is not guaranteed to work at all. Due to the standard, you are not allowed to cast an object to a derived class, if it is not part of that objects created inheritance structure. While it will work on a lot of compilers, if you compile gcc/clang with stric-aliasing, it will probably just not execute the calls at all.

JoeJ
JoeJ

Juliean said:
Please, for the love of god, never recommend this to anyone. I'm not one to judge preferenced, but this is plainly bad. At least use a static_cast:

Haha, well i have posted this actually hoping you would reply to correct me : )

I must admit i never use static_cast. Good practice is not worth the additional code here to me, but idk about eventual performance differences.

And sadly i do not like your second proposal either. Again more complexity just to be nice to the language.

I have no problems with clang either, but will pay attention to strict aliasing settings.
The UB argument worries me ofc. I did not know that.

It sucks, but i'll keep going this way, hoping future C++ opens a path i will like. And i'll avoid to use this for new code. I think the mesh is the only thing where i used this bad practice. : /

But thanks anyway! I can still change my mindโ€ฆ

Juliean
Juliean

JoeJ said:
I must admit i never use static_cast. Good practice is not worth the additional code here to me, but idk about eventual performance differences. And sadly i do not like your second proposal either. Again more complexity just to be nice to the language.

Performance is the same, any type of cast is a no-op essentially. It's not about โ€œbeing nice to the languageโ€ eigther (its actually more of the language being nice to you to even allow that - c++ is the only language where a constructor like yours could exist), but just not disabling all form of type-checking. Which, aside from UB, is actually what you are doing. What you are doing is essentially the equivalent of saying "I don't want to overload a function for different arguments, so I pass them as void* alongside the typeid:

void badMethod(void* data, std::type_info type)
{
if (type == typeid(int))
*(int*)data;
else if (type == typeid(bool))
*(bool*)data;
}

badMethod(โˆซ, typeof(int));
badMethod(&bool, typeof(int)); // yes, the mismatch is on purpose


That's how your example looks to me. You are of course free to do as you please, I mainly wanted to add in case of other people reading this and getting any funny ideas. No offence/don't take this personal, but the less code like the one you posted here exists, the better :D Many things are personal preference, that is just a very cut and dry case of bad code/design with no benefits and just downsides. The only โ€œadded complexityโ€ in my example is, idk, having to add a constructor and member variable? This is well worth not being an actual landmine waiting to explode on any type of refactoring.

EDIT: You wouldn't even need to add a constructor if you used curly-bracket syntax:

struct HEMesh_Modeling
{

// use m_mesh internally

int AddPolygon (const int edge,

const int atIndex = -1);

HEMesh& mesh;

}

HEMesh_Modeling{mesh}.AddPolygon();

At that point, you'd really have to explain to me what the โ€œadditional complexityโ€ compared to your example would be, because it's exactly the same amount of statements and less to type overall :D

EDIT2:

JoeJ said:
It sucks, but i'll keep going this way, hoping future C++ opens a path i will like.

What would even be a path you'd โ€œlikeโ€? Doing weird fake down-casts would not work anywhere that's not c++, so do you have something in mind that a different language offers? In C#, you'd probably have partial classes, but other than that, you'd also eigther have to pass the class as a parameter or store it in a field.

a light breeze
a light breeze

Use member functions in the following cases:

  • When you need to access private or protected members of a class (or expect you might need to in the future).
  • When you want to make the member function itself private or protected.
  • When you need to make a member function virtual (or expect you might need to in the future).
  • For some special functions that only work as members (i.e. constructor, destructor, some operators).

All other functions can and should be non-members. This includes the Update function in the original post.

JoeJ
JoeJ

Juliean said:
HEMesh_Modeling{mesh}.AddPolygon();

At that point, you'd really have to explain to me what the โ€œadditional complexityโ€ compared to your example would be, because it's exactly the same amount of statements and less to type overall :D

I like this curly version. I would not have thought to use this to do such nice one liner.

The complexity i meant is technically the need to create an object where no one is needed, and semantically the need to care about the member data which should not be needed either.

I can assure there is no potential landmine with my method. I use it for years already, it went through a lot of refactoring, but i never had any related problems, nor did i accidentally cast things to the wrong type. I wanted something easier to maintain, and it still is.

Thinking about it during the day, i concluded the best option would be to move modeling functionality to the base class. The other things are not so frequently used, and giving the mesh as parameter would be fine.

But then i thought i probably misunderstood your UB argument:

Juliean said:
Due to the standard, you are not allowed to cast an object to a derived class, if it is not part of that objects created inheritance structure.

I do not understand how a derived object could be NOT a part of the inheritance structure? It's the base class after all?

Juliean said:
c++ is the only language where a constructor like yours could exist

I do not use a constructor anywhere. And i can't follow your snippet to illustrate bad practice. To much keywords i don't know.

However, i do not try to write โ€˜proper C++โ€™. I'm just a C programmer who uses some C++ features. Casting stuff is no security hole to me. It's something i did all the time, and i see no point to change my habits if they never cause me problems.

Juliean said:
I mainly wanted to add in case of other people reading this and getting any funny ideas. No offence/don't take this personal, but the less code like the one you posted here exists, the better :D

Yeah no problem. Accepted and agreed. I won't do it again.

But i want to do some self defense. The key question is: Why not giving the mesh as parameter?

The answer has to do with the half edge data structure and its usage patterns.
For every little thing we need to traverse the data structure, since there are no index lists. (Getting rid of index lists is the primary motivation to use half edge - it makes life much easier.)
Writing those traversals is no problem, but reading them is difficult and requires much more focus than we want.
Adding a โ€˜mesh.*โ€™ to every line of code adds visual clutter, making the reading noticeable harder. It increases the time needed to maintain the code. Not only to me, but to anybody, i'm sure.

And that's the point where i prefer easier maintenance over good practice or quality of code.
That's no mistake. But if UB is a justified concern, sadly i have to find another way.

Alberth
Alberth

JoeJ said:
I do not understand how a derived object could be NOT a part of the inheritance structure? It's the base class after all?

In OOP, a base class is a sort-of skeleton for the derived classes. Derived classes add extensions on that skeleton. So every derived class is trivially also any of its base classes (recursively). A base class however is not a derived class because it's missing the extension.

In your case, you only add methods, and luckily for you the implementation of objects is such that no additional data is added to the derived class in that case, so it works. If the implementation would move the base-class data in a derived class (which I think it cannot do, at least not easily, so this is mostly hypothetical), or it would add eg a function pointer for the additional function of the derived class, your code will break.

JoeJ said:
Writing those traversals is no problem, but reading them is difficult and requires much more focus than we want. โ€ฆ

The mesh as you have it, is a pure data storage class with a handful of simple set/get functions.

Your derived class is adding โ€œeditingโ€ (a big word perhaps, but in essence, the derived class is not about storage but about changing what you stored. That's two different things). A rule in OOP is that a derivedThing must in essence also be a baseThing. Not just partly in implementation, but also in spirit. The โ€œspiritโ€ is the more important notion here, โ€œis-aโ€ should be taken very literally. In other words, โ€œclass HEMesh_Modeling : public HEMeshโ€ is wrong.

To get out of this, your idea to make the base class bigger is one way. Basically you switched idea about what the base class is. It's now an editor of stored data, and it needs all the editing functionality then. OOP is happy with that move with respect to inheritance, but depending on your view it' can be considered bigger than it should. No doubt some other part of OOP is less pleased now.

Another path is a generalization of what Juliean does. Make an EditMesh class with editing functionality to a, to-be supplied at runtime Mesh object. The editor class contains all the computing magic stuff to find the point in the mesh to change, and then modifies the mesh with the simple set/get functions.

A simplified version of that is a bunch of static functions that can do one kind of change. Depending on how much re-use exists between the different kinds of changes that may or may not be a good path.

Juliean
Juliean

JoeJ said:
I do not understand how a derived object could be NOT a part of the inheritance structure? It's the base class after all?

What I meant is similar to what Alberth said, but to explain it in more detail:

The type of an object-instance is that, by which you created it. If you create a โ€œHMesh", eigther on the stack or heap, that's its type. If you try to cast it down to a HMeshProcessing, this is UB. Because you did not actually create a โ€œHMeshProcessingโ€ instance, but a โ€œHMeshโ€. If you created a โ€œHMeshProcessingโ€-instance in your setup, you could cast it up to โ€œHMeshโ€ no problem, but the other way doesn't work. As explained multiple times, your code works incidentially on most compilers due to identical layout and what not. However, any other strongly-typed language would except on the cast.
Try that in C# or Java, you could not down-cast, because those languages always check the types of the objects, no matter what cast you use. So your code would lead to an exception in those languages. C++ just technically allows you to circumvent that, but again, only when reyling on UB. Which, btw, means that on a compiler where your code works, you will miss out on many potential optimizations that the compiler can perform, because it has to assume that code like you showed works (while a standard-compliant compiler can make many more assumptions about what is or is not possible in certain cases, leading to better code-generation).

JoeJ said:
I can assure there is no potential landmine with my method. I use it for years already, it went through a lot of refactoring, but i never had any related problems, nor did i accidentally cast things to the wrong type. I wanted something easier to maintain, and it still is.

Well, that's exactly like saying โ€œI went through a minefield 100 times, and never exploded, so I assure you its safeโ€, is it not? ๐Ÿ˜‰

JoeJ said:
The complexity i meant is technically the need to create an object where no one is needed, and semantically the need to care about the member data which should not be needed either.

Don't really see it. Creating an object like my example will be optimize out, so there is no runtime overhead. And I really don't see the actual technical complexity of creating an object to be any more then doing that case. Funnily, if we were dealing with primitive-types,

int(x);

is actually functionally equivalent to

(int)x;

So from a pure logical standpoint, there is no tangible increase in complexity in my example. It's just the correct solution for the requirements you want to achieve (split code from a class, but don't want to pass parameters to every method).

JoeJ said:
Adding a โ€˜mesh.*โ€™ to every line of code adds visual clutter, making the reading noticeable harder. It increases the time needed to maintain the code. Not only to me, but to anybody, i'm sure.

Well, actually no, not to anybody. As you have seen in the response of a-light-breeze, a lot of people go with โ€œeverything that can be a free function, should beโ€. That means actually passing whatever object you are working on as first parameter as the baseline. I'm not on that train myself, btw, but just your statement isn't really generalizable. I actually do not even have a problem with trying to avoid passing that parameter - quite the opposite, I also use member-functions unless I see a good case for a free function. I use my proposed solution in a lot of those cases, when I have really many functions in like a helper-library the work on an object. I'm just really โ€œoffendedโ€ by the misuse of inheritance and unsafe casts, you could say :D

JoeJ said:
However, i do not try to write โ€˜proper C++โ€™. I'm just a C programmer who uses some C++ features. Casting stuff is no security hole to me. It's something i did all the time, and i see no point to change my habits if they never cause me problems.

Well, let me ask you this then: If you could turn of type-checks in the compiler at all, like if you could make it accept the following code silently and then crash on execution:

void processMesh(Mesh& mesh);


processMesh(500);

Would that make your program and your coding experience more safe, or less safe? I think (read: hope) that your answer would be a resounding โ€œabsolutely more unsafeโ€. Which is pretty much what your cast does. It might not cause problems. I could give you a compiler-switch that allows you to do exactly what I just did here, and it would not cause you any problems at first - why would it, your code already compiled, so it will compile even without type-checks, no problem. It might not cause you any practical problems even in the future. Maybe you never mistype what object you are passing to what function. Maybe you never refactor code to a way where parameter-types change.
But the question is, would you not want your compiler to spot those things? Is it not a good thing that a lot of invalid code is already caught during compilation, and not just when running the code? That's my main point. It's not like your code causes segfaults as-is, but you lose the ability of the compiler determining that your โ€œmeshโ€ parameter has the correct type. It's as if you would make mesh a void* at that point, at that code.
If you think that's fine, so be it ๐Ÿ™‚ I also do my fair share of stuff thats UB or unsafe, but more locally in functions and class-implementations, not really in code that could be considered user-facing/API; which I'd consider your example to be.
Anyway, I think my point is made, and we got to agree on a few things, so that is already something :D

PS: Oh, I totally missed that. Seeing how you said you are a primary C programmer, do you maybe not know the difference between c-style casts in C++, and the actual C++-casts? If that's the case, this would kind of make what you are saying make a lot more sense to me. And could probably also make it easier for me to explain to you where I'm coming from^^

JoeJ
JoeJ

Juliean said:
The type of an object-instance is that, by which you created it. If you create a โ€œHMesh", eigther on the stack or heap, that's its type. If you try to cast it down to a HMeshProcessing, this is UB.

That's the point where i am unsure. (I do understand the derived also is a base, but not vice versa. Sorry about the clumsy question formulation.)
Probably i'm just desperate, but look at this:

class Base 
{
	int milk = 1;
	void Drink() {milk++;}
};
 
class Derived : public Base
{
};

void Main ()
{
	Base base;
	Derived ref& = (Derived&) base; // this can't be UB, since such cast does nothing yet. there is no unspecidief way to cast one thing to another.
	
	ref.Drink(); // this should be no UB either, since both classes are equivalent.
};

If i would try to read the C++ standard for some confirmation, i'd probably need a lawyer to figure it out.

But if the answer is โ€˜no worries, this is specified to workโ€™, then i would win the first round, and the next question becomes 'What if i move the Drink() to the derived class?'.

Juliean said:
However, any other strongly-typed language would except on the cast.

Sure, but that's the reason why i actually use C++. I want the low level hackery which came with C. It was low level enough to compete tedious machine code with a high level language. That's historically the reason why C++ is now the standard language for games, and it still relevant. Thus i'm not sure if specifications indeed declare things, which technically just work, UB.

Juliean said:
Well, that's exactly like saying โ€œI went through a minefield 100 times, and never exploded, so I assure you its safeโ€, is it not? ๐Ÿ˜‰

No it's not. Because i have exploded 100 times. I'm good at exploding, and i know what's common reasons, rare reasons, and never a reason like this. :D

Juliean said:
Creating an object like my example will be optimize out, so there is no runtime overhead.

The compiler might optimize the object away but there is no guarantee.
But i'm not worried about that, it's just that i would prefer to give as parameter, avoiding a need for extra awareness about object construction and member data.

Juliean said:
Well, actually no, not to anybody.

You make assumptions. Actually you can only believe my argument about the parameter making it harder to read, since that's all data you have. This is an exceptional case and problem, and general examples you provide are negligible to the case.

processMesh(500);

Juliean said:
Would that make your program and your coding experience more safe, or less safe?

Come on, the probability somebody would do such a mistake are tiny. But still, i would not turn off compile time type checks ofc.
It's not that i have fun with using hacks, nor do i think casting things is good practice. But sometimes it's the simplest and / or fastest way to get the job done. Like any tool, it has its applications.

But i can tell a related story. Recently i have changed my vector classes to accept only one number in its constructors. So i can make a vec(0,0,0) or Vec(1,1,1) easier. Same for a matrix to set its diagonal.
It's convenient, but it caused a kind of bug, similar to the things you expect here.
Writing stuff like

RenderCircle (5, pos, r,g,b);

can be affected, because the function actually wants more parameters: RenderCircle(radius, center, direction, colorsโ€ฆ)
It wants a direction, which i had forgotten. So it takes the red color and makes a direction out of it. This happens because the colors have default initializers. So it compiles now, previously i would have got an error message.

This kind of bug happens pretty often now to me. And it did not see it coming.
But: There are no good coding practices which would help, and those bugs are always very easy to find and fix.

Still, for the bad coding practice discussed here, such form of bugs do not happen.

Juliean said:
Maybe you never refactor code to a way where parameter-types change. But the question is, would you not want your compiler to spot those things?

That's a price i was consciously willing to pay when i cam up with this, and i did not regret it. Well, til yet at least.

Juliean said:
If you think that's fine, so be it ๐Ÿ™‚ I also do my fair share of stuff thats UB or unsafe, but more locally in functions and class-implementations, not really in code that could be considered user-facing/API; which I'd consider your example to be.

Yeah, i see it makes a really bad impression. It violates the standards people expect, and after the code works and no big changes are needed anymore, the advantage vanishes and confusion and doubts is all that's left.

That's what you guys have made clear to me. I did not think it would be that bad.

So i will continue using this until i'm out of this geometry processing shit-hole.
After that i will just use the derived modeling class everywhere instead the base class.
And i'll modify the other classes to take a parameter.
That's little work, and won't bother me at this point.

But i am a bit sad that i can not use my โ€˜creative method of grouping thingsโ€™ more often.
It's a bit like my father forcing me to cut my long hair, because โ€˜boys do not have long hairโ€™ :D

Thanks, guys!

Juliean
Juliean

JoeJ said:
But if the answer is โ€˜no worries, this is specified to workโ€™, then i would win the first round, and the next question becomes 'What if i move the Drink() to the derived class?'.

Nope, we don't even get so far. That cast is already UB. Yeah, c++ casting rules are quite complex. With the way your code works now, that c-style cast will use a static_cast, which is defined as the follows: https://en.cppreference.com/w/cpp/language/static_cast

If the object expression refers or points to is actually a base class subobject of an object of type D, the result refers to the enclosing object of type D. Otherwise, the behavior is undefined:

โ€œDโ€ here is the target of the cast. So โ€œactually being a base class subobjectโ€ means that it was created as โ€œDโ€, or any of it's derivates. In your example code, โ€œbaseโ€ is created as โ€œBaseโ€, which obviously doesn't derive from โ€œDerivedโ€, so even just casting to โ€œDerivedโ€ is technically invalid.

JoeJ said:
Sure, but that's the reason why i actually use C++. I want the low level hackery which came with C. It was low level enough to compete tedious machine code with a high level language. That's historically the reason why C++ is now the standard language for games, and it still relevant. Thus i'm not sure if specifications indeed declare things, which technically just work, UB.

Well the specification says what is UB or not. The compilers decide how to implement it. You are trying to make an inverse-reason argument here (because it works on some compilers its not UB). UB is defined in the standard; UB means compilers are free to do whatever they want (including considering this case as impossible to occur. At least, code reyling on UB is non-portable, and might have unexpected side-effects on different compilers, other versions of the same compiler, different settings, just differences in what cpp-file the code is put, whatever. If you want to still use that, sure (again I use my fair share of UB), but it's undeniable fact that C++ actually does not allow such behaviour and its totally coincidential that it works.

To show you a rather real example of what this can mean, the following code is a recreated example of some real-world code I once used in a cross-platform project:

https://pastebin.com/qS71CpJT

The

Juliean
Juliean

OH GREAT, THE GODDAM EDITOR CUT OF MY POST AGAIN. This forum software is my worst nightmare.

Continuingโ€ฆ

The codes use might only make sense in context, but you should get what it's trying to do. You use it as part of another method that can eigther accept a reference, or a pointer, and call a method if eigther a reference, or a non-null ptr is called.
That code compiled and ran fine on MSVC. Wanna know what happend on Clang? Crash. Strange, this code is safe, is it not? Nope. See, dereferencing a nullptr is UB. So Clang decided that, if I dereference the pointer in the method, the โ‰  nullptr must always be true. So there is no point in even checking the return-value, as the only possible (valid) return-value is "true". So it optimized out the condition, and always called the method, even when a nullptr was passed.

That's UB for you. That's what compilers are allowed to do when UB is involved. That's what relying on UB makes you vulnerable to. That's why its actually a literal landmine waiting to explode. When reyling on UB, you have no guarantees, and your code working is purely indicential. Even the same compiler-vendor might decide to change what your code does, without you noticing it, unless you run the code, which might then suddenly do nothing; crash; or worse.

You can say you are ok with that, but you cannot say that this is safe or good. You not running into problems due to stuff like this does not mean it's a good idea, and cannot cause problems at all. I also have legit cases where I depend on UB, but I'm certainly going to regret that if I ever go multi-platform.

I'm going to leave it at that. I feel like this conversion is just going in circles at that point, and we've hijacked the thread long enough already ๐Ÿ˜‰ What I've said about UB is the fact of C++ though, if you rely on such behaviour often then I'd argue you are not really using C++. Even C++ without UB still has it's fair share of cool low-level things you can do. It's not like wrong-casting objects is some core of the language that made it succesfull. But you do you, as long as you know the context and consequences of such actions (which you should do now) ๐Ÿ™‚

JoeJ
JoeJ

Juliean said:
OH GREAT, THE GODDAM EDITOR CUT OF MY POST AGAIN. This forum software is my worst nightmare.

I guess you know, but this works:
Write the post, not pasting any links in yet.
Copy your post to the clipboard.
Add the links and post.
If it crashes, try again, which due to the copy isn't that bad.

Usually i forget the bold part. : /

Juliean said:
dereferencing a nullptr is UB

Makes sense, but now i would not wonder if do this somewhere too. Good to know.

const auto [ref, isValid] = convertToCheckedRef(pToSomeObject);
if (isValid)
	ref.CallMethod();

I' curious how this works.
Does auto [ref, isValid] automatically convert the returned pair to ref and bool auto variables?
So you don't need to use the awkward first and second members of the pair?
That's really useful. (But it's also such things making modern C++ often hard to read for me.)

Juliean said:
Nope, we don't even get so far. That cast is already UB. Yeah, c++ casting rules are quite complex. With the way your code works now, that c-style cast will use a static_cast, which is defined as the follows: https://en.cppreference.com/w/cpp/language/static_cast

Hehe, although this is not the isocpp site, it's already harder to read than modern C++ code. :D
So i'll just take your word for it, but keep the tab openโ€ฆ
Remembering: โ€˜Although making a reference is guaranteed to do nothing, it is still a behavior, which in this case is undefined.โ€™

But now this raises me some deeper doubts!
For example, in code like this:

	__forceinline void FromAxisAndAngle (const sVec3 &axis, const sScalar angle)
	{
		sScalar half = angle * 0.5f;
		sScalar scale = sinf(half);
		*((sVec3*)this) = sVec3(axis * scale);
		setW(cosf(half));
	}

I cast the actual Quaternion class to a vec3, and i do this quite often with quaternions.
I was assuming it is my responsibility to ensure both classes have matching memory layout of its member data so this works.
I am aware memory layout may depend on HW, so there is potential UB.

But similar to the 'risk' when using unions, or assuming int is 32 bits, etc., i accept the UB here, knowing it should work on any HW i'll target.

Now i realize this kind of thinking is maybe outdated, and i should not do such things at all anymore.

Would it make a difference if i would use pointers instead references?

void Main ()
{
	Base base;
	Derived ptr* = (Derived*) &base // is this alrady UB too?
	
	ref->Drink(); // again i assume it's my responsibility, but should not be UB in a general sense
};

As i think of this, it should not even matter if there are derived classes involved in our discussion.
All that matters is that i cast things, taking the responsibility that they are equivalent in memory. And if can take this, that's usually the point from which i decide if UB is acceptable or not.

So maybe my real mental mistake here is that i forget about the difference of references and pointers?

Juliean said:
I'm going to leave it at that. I feel like this conversion is just going in circles at that point

I think your time is well spend. I can see the timebomb(s) meanwhile.

It's just hard to get rid of habits which have built up over decades, so i'm stubborn for some time.

Thanks! ; )

Alberth
Alberth

JoeJ said:
*((sVec3*)this) = sVec3(axis * scale);

Euhm, what is wrong with (assuming a โ€œdirectionโ€ field exists for the vector of the quaternion)

this.direction = sVec3(axis * scale);

JoeJ said:
I was assuming it is my responsibility to ensure both classes have matching memory layout of its member data so this works

I pondered about this during the day how to respond. Obviously if you don't do that things will break on mangled data, it's a necessary condition. It is however not a sufficient condition in C++, doing that is not enough to allow it for the standard. In fact there is no sufficient condition in C++. What you're doing is applying C semantics I think, except C++ is too rich in features to have such semantics.

JoeJ said:
Now i realize this kind of thinking is maybe outdated, and i should not do such things at all anymore.

Casting mostly exists as a last-resort if there is no other way to solve the problem. It's a blessing when you need it. The cost of it is that your friend the compiler will accept anything you say, even if it is complete nonsense or if at some other time you change something such that it becomes nonsense.

const auto [ref, isValid] = convertToCheckedRef(pToSomeObject);

The standard typically only states properties that you can expect, so implementations can pick any solution that works best for them.

The simplest form to support this is perhaps to generate

const Pair p = convertToCheckedRef(pToSomeObject);
const auto ref = p.left;
const auto isValid = o,right;

where the compiler will then likely eliminate the additional variables by replacing them by the right-hand side value during optimization.

JoeJ
JoeJ

Alberth said:
Euhm, what is wrong with (assuming a โ€œdirectionโ€ field exists for the vector of the quaternion) this.direction = sVec3(axis * scale);

I'm confused about your question. The posted code is part of the quaternion class, so there is no direction field, just x,y,z,w.
Actually i could add the vec3 direction as a member, making a union out of (float x,y,z,w) and vec3 dir. Because my vec3 internally uses 4 floats too, it would be memory equivalent and work. I would no longer need a cast to use vec3 functionality for the quaternion data, but iirc, using unions to rely on memory equivalency is some UB too.

Doing those things was totally common 20 years ago, but now i wounder how this may have changed.

Alberth said:
What you're doing is applying C semantics I think, except C++ is too rich in features to have such semantics.

Yeah. But i realize this only now.
I have always thought C++ is an extension of C. What worked in C, still works with C++, and that's guaranteed in specs. Beside some exceptions.
Butโ€ฆ that's wrong, i guess?

Alberth said:
Casting mostly exists as a last-resort if there is no other way to solve the problem.

I would say it existed before there were modern ways to solve it, so they must still work like they did back then.
But likely i'm wrong again. If static_cast was introduced with the first version of C++, then i just did not knew about that.
I have never ever used static_cast. Recently i've tried goto in debug code. It was a nice flirt. But static_cast? Why? I do not even know what it solves, because i don't care. My cast is nice. I'll stick to that.

Now you know. I have never read a C++ book.

Alberth said:
It's a blessing when you need it. The cost of it is that your friend the compiler will accept anything you say, even if it is complete nonsense or if at some other time you change something such that it becomes nonsense.

Hehe, yeah, that's the spirit! \:D/

That's really what i think of casts and how i use it.
But i don't want to be the last man on earth who does it either.

Alberth said:
The standard typically only states properties that you can expect, so implementations can pick any solution that works best for them.

Sounds a bit like Juliean has fixed the bug caused from UB with a hack, which luckily works due to UB? Do you mean this?
That's at least what i conclude from kind of dodging to simply say โ€˜yes, those edgy braces generate auto variables from pair membersโ€™.

But very likely i've got this wrong completely as well.
I'll put โ€˜read the bookโ€™ on my todo list. But it will remain at the very bottom. ; )

Alberth
Alberth

JoeJ said:
The posted code is part of the quaternion class, so there is no direction field, just x,y,z,w.

Ah right, my bad, I didn't realize there are dedicated quaternion data structures. It makes sense though ๐Ÿ™‚

In that case, I'd write a set_xyz function that simply copies the 3 values of the vector. An optimizing compiler will eliminate such a function, so no harm done.

JoeJ said:
Doing those things was totally common 20 years ago, but now i wounder how this may have changed.

Yep. Never liked the cast stuff though , it felt (and still feels) ieky. I like compilers telling me I made an error ๐Ÿ™‚

What changed is that C++ grew up (got improved to fit better to what is modeled). C only has functions and data, but everybody was writing a bunch of functions around a particular data piece. That then got formalized to objects in the very first C++ (reading how to implement objects and methods in C explains that step), and then further moves with constructor, destructor, virtual method, move semantics, etc. STL was added, Bare pointers were eliminated (at least for common cases) with uniq_ptr and shared_ptr. The entire look and feel of the language changed.

In other words, C++ expanded from C into the higher level code by adding features that allows you to manage large amounts of structured data easily.Raw computing speed is not the problem there, Managing the structured data is the main problem there.

JoeJ said:
I have always thought C++ is an extension of C โ€ฆ Butโ€ฆ that's wrong, i guess?

C++ being an extension is probably the reason why it works at all for you. Most of your casts wouldn't be allowed in other languages.

I somewhat wonder about the other direction; you seem to be writing mostly C-like raw computing code, why are you doing that in C++? Assuming you sometimes also have the high level structuring problem, you can use C code in C++ ๐Ÿ™‚ That is, make a bit more strict separation.

JoeJ said:
If static_cast was introduced with the first version of C++, then i just did not knew about that.

No, very first C++ was very much like how you'd do OO in C. It even generated C code and used a C compiler (and it still happens eg in Scala). Casting got fixed a long time after that. The fix is mostly about differentiating between what you want to change, and giving the compiler the option to approve it (which C cast semantics doesn't allow). Eg casting โ€œconstโ€ away or checked down-casting. Basically the compiler can understand my intention and check for me whether I do what I aim to do.

JoeJ said:
That's at least what i conclude from kind of dodging to simply say โ€˜yes, those edgy braces generate auto variables from pair membersโ€™.

The reason for dodging in the cpp reference is for allowing performance for all platforms. C++ is not tied to any specific platform or CPU. If they would say for example โ€œint is 32 bitโ€, then all systems with a different register width have a problem, and complying to the stated length means a drop in performance because those platforms must generate additional CPU instructions to make their integers obey the 32 bit behavior. So instead they say โ€œat least 32 bitโ€, and many platforms are much happier then.

So the standard never states such hard limits, not does it ever prescribe a single solution that all platforms must implement. I don't know what the reference says about [x, y] pairs, but I'd expect they say something like O(1) complexity, or so. Any solution with that complexity or less, is good. In that way, they give all platforms some room to manoevre to the sweet spot, and give manufacturers the possibility to be better than the competition in some way.

JoeJ
JoeJ

Alberth said:
Ah right, my bad, I didn't realize there are dedicated quaternion data structures. It makes sense though ๐Ÿ™‚

In that case, I'd write a set_xyz function that simply copies the 3 values of the vector. An optimizing compiler will eliminate such a function, so no harm done.

This makes me realize what i should do is starting to observe compiler output, which will help my doubts.
Sadly, being correct here means writing more code and more work on checking results. But those cases are quite rare at least.

But wait (still resisting), i have the perfect example:
I guess it's very common people use multiple math libraries in one project.
I have it because the physics engine uses it's own math lib, so i often have to convert forth and back to mine.
Knowing both libs use the same simd data types to build vectors and matrices, casting works, reduces code complexity, and ensures the compiler can't fail to optimize.

Would you still write and use conversation functions?

(Actually in this case i do use conversation functions, because physics engine uses vec4 with the last number telling if it's point or vector, but i use vec3 keeping the 4th. number unused. So i have to convert due to convention mismatch. But otherwise i would just cast, and i assume that's still common practice to many.)

Alberth said:
Raw computing speed is not the problem there, Managing the structured data is the main problem there.

Agree, but personally i tend to use higher level language features only for higher level things.
At the bottom, where raw computing speed is the problem, i mostly still prefer C style.

This is not only a philosophy i like, there also is a lot of paranoia regarding hidden costs from high level language features.
I really love lambdas for example, but i noticed the convenience can come at a cost. Same for templates.
But the worst thing seems constructors. I still mostly use default constructors which do not initialize the data. Back then this was recommended often.
Some years ago i have noticed my math lib is 10 times faster than glm. Glm is well written C++ code. It's not written to be super fast, but it should not be that slow. Tho profiler has shown glm constructors pretty much on top. It seems the compiler completely fails at optimizing them away, or it just implements them inefficiently. Did not compare assembly output.

It's hard to change my mind when the reality diverges so far from the programmers ideals and assumptions.
I might accept a performance loss of <5% for โ€˜better practiceโ€™ eventually, but not more, and never 10.

Alberth said:
I somewhat wonder about the other direction; you seem to be writing mostly C-like raw computing code, why are you doing that in C++? Assuming you sometimes also have the high level structuring problem, you can use C code in C++ ๐Ÿ™‚ That is, make a bit more strict separation.

No, actually i feel like mostly writing high level code. I do not even optimize. I care about time complexity, for the rest i just try to write โ€˜fastโ€™ code from the start by knowing about the HW and assuming the compiler does a good job.
I'm not up to date with modern C++, but i rely heavily on some C++11 features to reduce code complexity. Going back to just C would be a nightmare.
So i'm not a low level purist - it's the last resort. But i'm stuck in a 90's mindset maybe, because i never learned about paradigms, modern ways, or general changes and progress regarding programming. Because i did not have related problems, i rather invested my learning time into math for most.

But i was wondering about combining C++ and C just out of curiosity. How could i tell the compiler which h file is C and which is C++? Are there different compilers at work if i used both languages, or is it technically all C++ then anyway?
Maybe there is some optimization merit here, since i see reports claiming C is almost two times faster than C++ (which i kinda doubt).

But as long is don't have any performance problems where i could identify a single major bottleneck to be the cause, that's just a rhetorical question. And i feel those times are over. Modern software is just so complex we almost never see such single bottleneck worth to optimize the shit out of it. That's why i try to write fast code from the start, because i'll probably never get back to it once it works.

It's very different on GPU though. Here subtle optimizations still make a big difference, and low level thinking still applies to me.
But because they failed at making GPU available to general programming, i rarely do this, and rather accept the slower CPU for code which is actually maintainable.

Alberth said:
Eg casting โ€œconstโ€ away or checked down-casting. Basically the compiler can understand my intention and check for me whether I do what I aim to do.

I can imagine a bit what advantages i'm missingโ€ฆ : )

Thanks for the feedback! Other programmers thoughts are very helpful. It serves me as a replacement for โ€˜reading the bookโ€™. : )

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.