How to control if const or non const Getter is called?

Started by
11 comments, last by JoeJ 1 year, 7 months ago

I'm currently trying to reduce redundant disk writes from my editor, which stores it's objects in some storage class.
So i decided to add a flag to the storage. If the storage goes offline, it is saved to disk only if the flag is set, otherwise it's just deleted because i know the file on disk is still up to date.

To avoid a need to go through all code and setting the flag on any changes, i came up with this idea (code is member functions of a StorageManager class):

		const SceneNode* GetNode (const Link link) const
		{
			const DynamicStorage* storage = GetStorage (link); 
			if (!storage) return NULL;
			
			// not setting the flag. Becasue we get a const pointer, there won't be changes to the node
			
			if (link.index<0 || link.index>=storage->nodes.size()) return NULL;
			return &storage->nodes[link.index];
		}

		SceneNode* GetNode (const Link link)
		{
			DynamicStorage* storage = GetStorage (link); 
			if (!storage) return NULL;
			
			storage->hasChanged = true; // <- setting the flag here, because if we get a non const pointer, we likely do some changes to the node 
			
			if (link.index<0 || link.index >= storage->nodes.size()) return NULL;
			return &storage->nodes[link.index];
		} 

See the comments to follow my thought. Let me know if you think that's a bad idea in general.

I think it should work, but it doesn't.

If i do something like this:

const SceneNode *node = GetNode(link);	

The non const getter is called, so the flag is accidently set. Although i try to make clear i want a const pointer.

What do i miss here? I never payed attention to this language detail before.

I guess i could solve it this way:

const SceneNode *node = ((const StorageManager*)this)->GetNode(link);

To make clear not only the returned pointer is const, but also the Manager.
But then i'd need to go through all code again, which is what i've hoped to avoid.

Advertisement

I think this is probably a bad design if you run in to these kinds of problems. This partially solves the serialization issue, but won't help in other places where you need to do proper edit notifications. For instance, if you edit an object it might need to show the change in a different editor window, but your current design won't support that without constantly polling.

The way I do this in my editor is to call an event-handler function after an object is edited. Its signature looks something like this:

void editedResource( resource, editFlags, const void* sender );
// editFlags: what is edited (DATA, METADATA, NAME, URL, etc)
// sender: used to avoid an editor responding to an event it created.

This function is passed around as something like a std::function<>, and can be called from any editor GUI. This function propagates the edit event down the editor GUI hierarchy, so that any editor can respond to the event. The function is also a global place that can be used to mark resources that need to be re-serialized later. This works well and is not a performance bottleneck as long as edits are not constantly happening (i.e. rather than calling the function throughout a continuous edit gesture, only call it once at the end).

Somewhat unrelated, but you also need some way to handle undo/redo. I use the Command design pattern for this, and maintain a stack of undo commands. Undo commands also have ability to call the editedResource() function when undo() or redo() are called, to propagate the edits to the editors viewing the resources.

So a typical continuous edit operation looks like this:

void startEditSlider( Slider& slider, double newValue )
{
	// push undo command onto stack.
	undoStack.add( EditResourceCommand( resource, editFlags ) );
}

void editSlider( Slider& slider, double newValue )
{
	// apply edit to resource.
	resource.value = newValue;
}

void endEditSlider( Slider& slider, double newValue )
{
	// call editedResource() notification function
	editedResource( resource, editFlags, this );
}

For a non-continuous edit (i.e. pressing button), those functions get merged into one, starting with undo handling, applying edit, then notifying other editors.

Aressera said:
constantly polling.

That's indeed what i do. : )

Aressera said:
Somewhat unrelated, but you also need some way to handle undo/redo.

I handle this by making a copy of the files manually, in case i plan to do something which i might eventually regret later :D

You see my ‘editor’ is quite basic.
But it's not really a game editor, just a procedural modeling tool for static scene. The number of objects goes up to billions, so all that matters for now is making processing and out of core management as fluid as possible.
I'll care about manual edits later, adding the things you mention on top.

Regarding my question, i realized it's pretty stupid. The compiler does not choose functions based on return type, so my idea can't work.
I'll rename both functions and then go through error messages, deciding everywhere if changes might happen or not. : / On the long run that's better because it's verbose about the otherwise hidden detail.

But thanks anyway!

You should probably rename the non-const getter. Calling a getter should always be a read-only operation.

a light breeze said:
You should probably rename the non-const getter. Calling a getter should always be a read-only operation.

Yeah, that's what i did. Error count still above 100. Guess this will cost me the whole day…

I have to admit i often use ‘getters’ also to return non const references so i can change the data. Probably i should not use ‘get’ at all to avoid confusion with established assumptions about getters and setters.
In general i'm too lazy to make getters and setters for everything. But i rarely miss not having the encapsulation advantage.

JoeJ said:

I think it should work, but it doesn't.

If i do something like this:

const SceneNode *node = GetNode(link);	

The non const getter is called, so the flag is accidently set. Although i try to make clear i want a const pointer.

What do i miss here? I never payed attention to this language detail before.

I guess i could solve it this way:

const SceneNode *node = ((const StorageManager*)this)->GetNode(link);

To make clear not only the returned pointer is const, but also the Manager.
But then i'd need to go through all code again, which is what i've hoped to avoid.

This doesn't work because this is not const in your function, and beyond that that is not how const correctness works in your application. You could add a Tag to a function that will work and gives you selectable behaviour.

a light breeze said:

You should probably rename the non-const getter. Calling a getter should always be a read-only operation.

This is not true specially not is you look at the STL. Vectors data and at functions come in const and non const flavours. In general your getter shouldnt return a non const value but the is really depends on the object itself and its semantics.

Worked on titles: CMR:DiRT2, DiRT 3, DiRT: Showdown, GRID 2, theHunter, theHunter: Primal, Mad Max, Watch Dogs: Legion

JoeJ said:
Although i try to make clear i want a const pointer.

C++ has lots of subtle const notions ?

I know const pointer as “"int * const p” (cannot change p but you can change *p). “const int *p” is a pointer to constant data (cannot change *p but can change p). Obviously, “const int * cont p” exists too in case the initial value is the best forever. See also https://stackoverflow.com/questions/21476869/constant-pointer-vs-pointer-to-constant

The “f() const” of a member function is about not modifying data inside “this” (modulo the “mutable” notion). It is completely unrelated to constness of the returned value.

I am not sure what the cpp call resolve rules say exactly, but at the moment you need to insert a ((const StorageManager*)this) thing, it's likely simpler and more clear to use a different function name instead, as others have suggested. It might uncover a few spots with a “weird” call as well.

NightCreature83 said:

a light breeze said:

You should probably rename the non-const getter. Calling a getter should always be a read-only operation.

This is not true specially not is you look at the STL. Vectors data and at functions come in const and non const flavours. In general your getter shouldnt return a non const value but the is really depends on the object itself and its semantics.

There are some major differences between STL const/non-const overloads and the original poster's situation:

  • The STL functions actually are pure getters that do not, in and of themselves, modify any object. The returned reference can be used to modify the object, but that's a separate operation.
  • The STL functions have no semantic difference between the const and the non-const version; the only difference is the constness of the returned reference. auto const &x = v.at(0) does the same thing whether v is const or non-const.
  • The STL functions are not called Get<something>.

JoeJ said:
Let me know if you think that's a bad idea in general.

It's say it's a code smell of bad design.

It seems like you mixed the responsibilities of reading, writing, and monitoring for differences. Instead reading should be a specific action. Writing should be a specific action. If you want to keep a clean copy with a dirty flag, that would be independent.

I'd build it so reading gets a copy of a structure. Saving accepts a copy the structure. You might decide to have a dirty flag in the structure, or keep a pristine copy that you compare a save against, whatever works better for your data. Then there is no ambiguity.

frob said:
Instead reading should be a specific action. Writing should be a specific action.

This is the first time i work on out of core processing. Many things i'd usually do seem no longer possible. I no longer have a load and save, instead i have a sync operation to ensure changes made in memory are reflected on disk while stuff goes offline.

Because i can't have the whole scene in memory, a typical processing step works like so: Load a tile, process it (e.g. doing erosion simulation), move to the next tile. As the active range moves from one tile to the next, saving changes and loading new scene parts happens in the same function.

I can also not have multiple copies of the scene just to support something like undo. I have one copy so i can do background processing with multiple threads, but i could not effort more than that.

It's rather strange. Basically i have to treat disk like RAM, but then i have nothing to get the functionality we usually associate with disk storage.
At some point i need to enable this using file backups, mainly because a crash would leave disk data in an undefined state. But for now i'm happy it indeed works the way i have planned it.

I'm also happy my lazy idea to misuse const correctness did not work. While renaming functions and going through all the code i found quite a few corner cases which required some thoughtful changes. So if it had worked, i'd have introduced subtle bugs.

This topic is closed to new replies.

Advertisement