Sign in to follow this  
VanillaSnake21

Handling resource pointer with RAII

Recommended Posts

I'm int the process of rewriting my bitmap class to better support the RAII idiom, however I've got some issues/questions. First of all this is my class so far


class BitmapFile
{
public:
	BitmapFile(string fileName);
	~BitmapFile();
	BitmapFile& operator=(const BitmapFile& rhv) = delete; //prohibit copying of this RAII object

	void FlipBitmap();
	void ResizeBitmap(RECT resizeRectangle);
	void ResizeBitmap(float xscale, float yscale);
	void RotateBitmapLeft();


	POINT GetDimensions() const;
	int   GetDataSize() const;
	const BITMAPFILEHEADER& GetFileHeader() const;
	const BITMAPINFOHEADER& GetInfoHeader() const;
	const UCHAR* GetData() const;
	const PALETTEENTRY* GetPalette() const;


private:
	int LoadBitmapFromDisk(string szFileName);
	int Unload_Bitmap_File();

	BITMAPFILEHEADER fileHeader;
	BITMAPINFOHEADER infoHeader;
	PALETTEENTRY	 palette[256];
	UCHAR*			 data;


};

Now the problem here is that the pointer to data is still not encapsulated well enough since I have the GetData() function. So some code can potentially call delete on that pointer. However at the same time I still need that flexibility of for example being able to create a bitmap at runtime not just load it from a file. So how can I get this to fit into RAII principles correctly?

Share this post


Link to post
Share on other sites

Now the problem here is that the pointer to data is still not encapsulated well enough since I have the GetData() function. So some code can potentially call delete on that pointer.

 

This has nothing to do with RAII per se. What you are describing is a user deliberatly doing something he is not supposed to do. In most cases, there is no reliable protection against this. Some could coul potentially do:

BitmapFile file;

delete &((char*&file) + sizeof(BITMAPFILEHEADER) + sizeof(BITMAPINFOHEADER) + sizeof(PALETTEENTRY));

And no amount of encapsulation can protect you against this (this exact code won't necessarily work, but you get the idea).

 

RAII should most of all help you reduce the case where data is accidentially deleted, not deleted at all or double deleted. So if your entire code relies on RAII, there is little need to call delete in the first place (you should really be using std::unique_ptr or such for storing data, ie.).

 

The point is: You should protect against easy to make mistakes, not delibarate attempts to screw your code. Why would someone delete a random pointer they aquired from somewhere? Thats why you should use RAII and smart-pointers (std::unique_ptr is a 1:1 replacement for new/delete, without any real drawbacks) in the first place, then when you get const UCHAR* from a function, you know that you are not supposed to delete it.

 

Now whether the encapsulation of this class can be improved regardless is another topic, but has nothing to do with RAII and/or protecting against forced memory deallocation.

Share this post


Link to post
Share on other sites

You should make a copy of the bitmap data, and give out that copy.

 

That leaves you with just one hole

BitmapFile file;

memset(&file, 0, sizeof(BitmapFile));

Good luck protecting yourself.

 

 

Now take a step back, and consider what you're doing.

You have two (or more) objects in one program. Each object has a specific task. Other objects need to have that task available to do their own task.

It's not each object for itself, and screw everything else. It's a co-operative exchange of data and using services so together the objects reach their objective. Objects need each other, they are friends of each other, even if they have no 'friend' declaration.

 

Nobody will write code like above. If they do, they are killing a service they rely on, and thus also kill themselves. For the same reason, nobody will delete your data if you spread the word they shouldn't do that.

 

Many scripting languages really embrace this idea of cooperating objects, and simply discarded 'protected' and 'private'. Everything is public. It's very scary at first, but it does make sense.

 

In fact, I copied this idea to my other languages, like Java and C++. Almost everything is public, there is only one rule: Do not mess with my data, unless I say it's ok. Private data is usually internal administrative data which is useless to other objects.

This means everybody can read anything, I don't need getters, mostly. I do find that I write getter-like code here and there because the requesting object needs the information in a slightly different form than it is stored, or they need a sub-set of it, or some property about it. I do have setters, although these usually live at a higher abstraction level (ie "add something", which then involves changing several data values at the same time).

 

I do introduce getters if the need arises. At that point, I look at who uses the data that I am about to change, and in what way. This is not really different from checking how other objects use your getter method, I think, when you are deciding whether to keep the method.

 

Share this post


Link to post
Share on other sites

@Juliean I wasn't sure how far I should go to protect it, I was getting the impression that the forefront of RAII was to avoid loose pointers that get passed around with things like GetData(). So I mean I could completely encapsulate the data entirely, but then I would have to make give this class it's own draw function and basically make it a stand alone class. So you're saying it's ok to leave it as is as long as I encourage RAII practices overall in my code and discourage use of manual alloc/dealloc by the user? 

 

Another question how would I still use RAII in this principle as I almost certainly will need to create a runtime bitmap. I have a sprite loader which takes a large bitmap and spits out small ones that have one cell per bitmap, so I need to allocate data and init the headers, is there a safe way of doing this?

 

@Alberth Well actually I was kind of using the same principles you speak of right before I started the rewrite. I had a BITMAP_FILE struct with headers and data completely public, and just had loose functions like LoadBitmap and UnloadBitmap which worked with that struct. It was working pretty good, however when things got a little more complex, that is I wrapped the BITMAP_FILE into another class that handled Animations, then things started getting out of control and I got stack corruptions at certain points. It wasn't due to the bitmap, but in any case that's where I spent most of the time looking since I knew I had these loose pointers everywhere. So when things go south I think it's helpful to know that at least it can't be these parts to blame.

Share this post


Link to post
Share on other sites
@Juliean I wasn't sure how far I should go to protect it, I was getting the impression that the forefront of RAII was to avoid loose pointers that get passed around with things like GetData(). So I mean I could completely encapsulate the data entirely, but then I would have to make give this class it's own draw function and basically make it a stand alone class. So you're saying it's ok to leave it as is as long as I encourage RAII practices overall in my code and discourage use of manual alloc/dealloc by the user?

 

No, no, you still have to pass pointers (or preferably references) somewhere. Even if you encapsulate your base data, at some point you have to pass ie. your BitmapFile-class per pointer.

 

On a general note (not on the count of RAII), it can be beneficial to increase the amount of proper encapsulation. Its always good if you can avoid passing your external data-members, but on the other hand you also want to avoid your class doing too much. If you can have a slim draw function that takes a few parameters for configuration and handles the actual bitmap submissing/drawing interally, then thats probably fine & better than just having a public accessor (again, not necessary due to RAII but just the general concept of data hiding).

 

Alternatively, you can use a variant of the observer pattern, ie. if you have some GraphicsDevice-class, you can do:

class GraphicsDevice
{
public:
    void DrawBitmap(char* data);
};

class BitmapFile
{

   void Draw(GraphicsDevice& device)
   {
        device.DrawBitmap(*data);
   }
};

Then you don't have to expose the data member, and don't overload the bitmap class with too much functionality. Thought this introduces some coupling between the BitmapFile-class and your render code, which means you have to consider the tradeoff in your system. Maybe someone else can comment on that, too.

 

 

 

In fact, I copied this idea to my other languages, like Java and C++. Almost everything is public, there is only one rule: Do not mess with my data, unless I say it's ok. Private data is usually internal administrative data which is useless to other objects.

 

That actually sounds nightmarish for me as a user of your code. How do I know what I'm supposed to call or not? Working in an IDE with autocompletion, when working with 3rd-party code I always look at the suggestions that I get (which are only public members). So if I have an Actor*, and I type  ->Position, I expect to get some methods that allow me to modify the position of the object. If I get an suggestion SetPosition, I can safely assume that this is what I can call to modify the actors position, without blowing up. Now in your case, how do I know what I can call or not? Maybe I get Actor->vPosition, can I modify that or can't I? Now you said "do not mess with data unless I say its ok". So do I know have to lookup every elements documentation to see if its safe to call? That sounds like a huge hit on productivity (yes, you have to read documentation one way or the other, but for very simple things Like UActor->SetActorLocation, I can just look at the signature and see that I can pass i an FVector3, and it will safely move the actor and all its childs & components to the specified location. If I got the option of calling all the internal data of UActor, it would be way harder to say whats the correct way of doing things without fucking stuff up).

 

Thats kind of the point of protected/private: Safety. Private, in languages that support it, should be the default way. Instead of asking, "Do I need to make this private", you should ask: "Do I need to make it public?". I mean in all honesty, whats the point of having everything public if someone using the class is not supposed to access it anyways? Maybe I misunderstood your intention, but if you cannot safely modify a data member of an object or call the function without further setup, it should be private, PERIOD. The reason why scripting languages don't have it most of the time is probably that they are factored more towards getting quick results and not building entire complex systems like an entire game-engine in python (at a company I worked for, the actually did this, and they made up their own naming convention to show what was supposed to be private and whats supposed to be public in their python code).

 

But maybe you meant something different, it just sounded kind of extreme, and I didn't want to leave that without at least a comment :)

 

EDIT: Actually if you are just talking about making private values with a Setter/Getter-pair public instead, thats not half as bad. Recently I've still been going more towards setters/getters, as those are easier to refactor by 1) allowing to selectively remove eigther read or write accces, 2) make it easier to change the internal representation/naming of the value, 3) make it easier to add buisness logic (verification, calculation of the value from other values, ...) later on, etc...

Edited by Juliean

Share this post


Link to post
Share on other sites

Now in your case, how do I know what I can call or not? Maybe I get Actor->vPosition, can I modify that or can't I? Now you said "do not mess with data unless I say its ok". So do I know have to lookup every elements documentation to see if its safe to call?

indeed, treating my classes as black boxes won't work. You need to have understanding of what each class does what its properties are, and how the class and the data fits between the other classes. This is knowledge you need to have anyway, if you're making a non-trivial change. Note that unlike what you may think, free write access is only for really simple data that has no links to other parts. One example would be colour. Any property that influences game play (like position in your example) is normally set with a function, as it typically needs additional processing, or notification of other objects/systems.

 

 

This means you must read the documentation before you code. I am not sure how this is bad. Your auto-completion fails, making it very clear what you're doing is not working. After a few days working in some area, you know all the patterns just like you know the getter/setter function names after a few days.

 

The main use of public data is read access. If you want to know the position of an actor, just read actor->position. I fail to see how "def GetPosition(self): return self.position" does anything useful.

 

 

If I got the option of calling all the internal data of UActor, it would be way harder to say whats the correct way of doing things without fucking stuff up

Being able to call or use something doesn't mean it's a good idea to do so. Making things less open doesn't really solve much. Often objects expose several methods that are intended only for some other objects (eg "update" which is supposed to be called only from a higher level object rather than from arbitrary code). So even in the world where you limit access with protected/private, you still have to watch what you're doing, you can't just blindly follow your auto-completion, nor just write somewhere without checking you're allowed to.

 

 

I mean in all honesty, whats the point of having everything public if someone using the class is not supposed to access it anyways?

Deciding what data may or may not be seen for other classes that don't exist yet, is pure speculation. You're making decisions that may not hold at all, how do you know what you decide is correct?

 

 

For some data it is very clear that nobody will ever need it. Typical examples are internal handles etc. This kind of internal administrative data, I do hide.

 

There is however also a big set of "well, I don't know really". I see no harm in giving free access to all that data (ie the set "not supposed to" is empty, except for the internal administrative data). Unlike in the real world, all objects live together in a co-operative program, working together. There are no criminal objects that try to break things, you don't need police, guard dogs, or big fences to make all objects behave nice to each other. Everybody believes you do, because the OOP literature needs to sell you the various forms of access control. It exists for some very good reasons (see near the end below), but I don't see why you need the same big walls everywhere, in particular in code where objects cooperate and trust each other.

 

 

 

Maybe I misunderstood your intention, but if you cannot safely modify a data member of an object or call the function without further setup, it should be private, PERIOD

Why? Is it so hard to avoid writing  "actor->position = ..." otherwise? I mean, "actor->position =" is pretty obviously writing outside your own data space, right? Does it really need a big fence, and a police dog to stop you from doing that?

 

 

EXCEPTION:

There is one big exception to all this. If you make a library where you want to be able to change internals, without breaking all uses of the code, then yes, you want a full interface with all the getters and setters and stuff, and everything else must be hidden.

 

But I don't see why you need the same kind of protection between objects in the same program, where you have full control over all source code. The example of the OP, where a full wall is created, because oh dear, the caller might do something I don't want. The BitmapFile object will survive, except unfortunately, the program as a whole will collapse, taking down the BitmapFile in its fall, any way. What's the use of that wall?

 

@Op: No offence intended

Share this post


Link to post
Share on other sites

God damnit, I just lost my long post. I'm not motivated to retype it all, so I'll keep it short.

 

I think I'll just sum up what the main problem is: You keep thinking that designing an API where a lot of possible calls that the user can make are actually invalid, and won't result in a compile-error or handle-able error, but just in unexpected behaviour. Not only does this go against the principle of least surprise, where every procedure should do exactly what you can expect of it. Its just a wrong approach in my opinion, you shouldn't make it easy for the user to fuck up if he doesn't follow your exact idea about the code, but you should make it hard to fuck up for the user if he just follows what the API allows him to do. Thats why we have const-correctness, thats why we have RAII, thats why we pass by & instead of *, etc, etc.

 

In all honesty, would you rather work with an API where you

A) Can assume that almost every public method/data-member can be called and written to, without unexpected side results, if you adhere to the general function signature.

OR

B) Can assume absolutely nothing about what is an valid call or not. If you want to write a value, unless you know the library at heart, you have to make sure that writing to it is actually valid.

 

And yeah, if you begin making read-only members public because you don't feel like writing a getter (pure setter/getter pairs as I said can be omitted in most cases), then the actual worse thing that happens is, that a user cannot assume anthing. You are not designing by contract anymore, you are designing by "you have to know exactly what you are fucking doing, otherwise you are screwed!".

 

I actually lost my whole detailed answer to your points, but here I also realized that it pretty much doesn't matter anway since every point I would issue, because every concern about safety seems to just result in "why does it matter" to you? But anyways, here is some details:

 

- I actually recall a bad situation in Unreal, where you could both set a bReplicated-member, as well as Call SetReplicated. SetReplicated performs additional work, and setting bReplicated = true doesn't work outside of the constructor. This lead to some nasty replication bugs (stuff just wasn't replicating and we didn't know why), which resulted in an extremely long, pointless bug hunt. As for your question "Is it really hard not to write bReplicated = true"? Yes it is, if you haven't designed the library yourself or worked with it a tremendous amount of time. It shouldn't be possible to call bReplicated = true, if it doesn't work, PERIOD.

 

- Getter-only access does another thing for pointers. If a raw pointer is a public member, users cannot only modify the pointers content, but also the pointer itself, which most of the time is probably not what you expected. Having char* GetData() allows a user to write to the pointer, without changing it.

 

- This leads to an important point, consistency. If you adhere to strict access rules, it is imminent how to handle values. If I have a public pointer member, I can set it, not matter what. If I have a public property, I can set it. This not only reduces space for errors, but also increases productivity. Yes, having to check the documentation for every single data member/function is a very bad thing. No-one will remember what is allowed to be accessed and what not without a huge amount of time spent.

 

- Actually I'm kind of beginning to feeling a tad bit shizophrenic with this whole discussion. Access-modifiers have been invented to restrict access, and you are just like "some of my publically accessibly data members are actually not fully publically accessible". ...

 

- The point about future extention of a class.

Deciding what data may or may not be seen for other classes that don't exist yet, is pure speculation. You're making decisions that may not hold at all, how do you know what you decide is correct?

 

 

I'd put it the other way around: You design your classes with a clear purpose in mind. SOLID-principles in mind, you design your classes with a single, well defined responsibility, and make it open for extention, but close for modification. That way, future classes shouldn't need to access data other than what you expected - you just designed your class to handle itself in terms of mostly input/output, and not state. Well thats another point per se, but worrying about future classes uses is a strong case of YAGNI.

 

Well as I said as far as I can see, your point boils down to "Its absolutely fine to have an unsafe class API", to which I pretty much can only say "no, its not", for all the points I listed. Yes, its not possible have eigther extrems (a rock-solid vs. a totally unsafe API), and in reality you have to take trade-offs, but you should definately aim towards making your API safe for use per se, instead of "requires intensive knowledge to be safe". 

Though you are kind of putting me off by at one point saying you are hiding class internals, and then on the other hand saying that its ok to have some internals write-accessible even though unsafe (like position, which is dependant).

 

Though I'm laking a lot of work-experience, I'd be glad to hear what some other professionals have to say about this. Though I feel this is drifting away from what the OP originally wanted to know, so maybe this should be moved to its own thread, if the discussion should continue.

Edited by Juliean

Share this post


Link to post
Share on other sites

All right, thank you, I'm going to keep it this way then.

Sorry for derailing your topic.
I think what you have should be fine. There will always be holes in them, being aware of them is the first step, and often also the last step.
 
@Juliean: I think we'll need a lot more than one topic, I am not even sure we're talking about the same thing.
I also agree this is not the place or time.

Share this post


Link to post
Share on other sites

Another question how would I still use RAII in this principle as I almost certainly will need to create a runtime bitmap. I have a sprite loader which takes a large bitmap and spits out small ones that have one cell per bitmap, so I need to allocate data and init the headers, is there a safe way of doing this?

 

This seems like a clear sign that your BitmapFile class is doing too much. Bitmaps don't always come from files and the two concepts should be handled separately. If I were designing this feature, I'd make a Bitmap class that stores a chunk of pixels and has operations to draw those pixels on screen, and another class that knows how to read files and extract the bitmaps they contain. That way you could create bitmaps from other sources without being tied to files or file formats.

Share this post


Link to post
Share on other sites

 

Another question how would I still use RAII in this principle as I almost certainly will need to create a runtime bitmap. I have a sprite loader which takes a large bitmap and spits out small ones that have one cell per bitmap, so I need to allocate data and init the headers, is there a safe way of doing this?

 
This seems like a clear sign that your BitmapFile class is doing too much. Bitmaps don't always come from files and the two concepts should be handled separately. If I were designing this feature, I'd make a Bitmap class that stores a chunk of pixels and has operations to draw those pixels on screen, and another class that knows how to read files and extract the bitmaps they contain. That way you could create bitmaps from other sources without being tied to files or file formats.

 

 
Uh...
 
So loading the resource is not the responsibility of the resource class, but rendering it is? Seems a bit silly.
 
The accessors for header data are the part that worries me.
 
Also, @Snake, RAII for a dynamic array is std::vector.
 
Oh, and...
 

BitmapFile& operator=(const BitmapFile& rhv) = delete; //prohibit copying of this RAII object

BitmapFile(const BitmapFile&) = delete; //also disable copy construction
BitmapFile(BitmapFile&& rhs) = default; //and enable move construction - default will work with vector but not UCHAR*
Edited by Khatharr

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this