It is currently Sat Aug 08, 2020 8:26 pm


All times are UTC




Post new topic Reply to topic  [ 4 posts ] 
Author Message
 Post subject: constness of *SurfaceExtractor volume argument
PostPosted: Wed Dec 21, 2011 9:54 am 
User avatar

Joined: Wed Jan 26, 2011 3:20 pm
Posts: 203
Location: Germany
This is not really a bugpost, but it feels like a bug to me :)

The surface Extractors take a pointer to a volume.
I'm assuming that the Extractors will only call const functions of the volume object (since the extractor should not change the content of the object, I'm aware of the internal mutables)

also pointers are so '99 ;) and my c++faqs tell me I should use references where I can, pointers where i must.

Does anything speak against making that volume pointer argument
1. const
2. a reference


Top
Offline Profile  
Reply with quote  
 Post subject: Re: constness of *SurfaceExtractor volume argument
PostPosted: Wed Dec 21, 2011 4:30 pm 
Developer
User avatar

Joined: Sun May 04, 2008 6:35 pm
Posts: 1827
ker wrote:
Does anything speak against making that volume pointer argument
1. const

I think it should be const, but actually I just gave it a quick test and it doesn't compile. The problem is that the Volume::Sampler also needs to be fixed for const correctness (and then there may be further changes). This should be done, but means it's no longer a 5 minute job. I'll leave this bug open until this gets sorted.

ker wrote:
2. a reference

Not sure about this one... but it probably should be changed. I'll need to think about it some more. One point of concern is that we now have the start of some image processing classes (VolumeResampler, LowPassFilter, etc). More will probably be added, and it is possible that it will be nice to chain them together and then execute the whole chain at once. Toolkits such as ITK use this approach. I don't know yet if it makes sense for PolyVox, but if it does then using pointers may make it easier to build such chains. We'll have to see how that develops.


Top
Offline Profile  
Reply with quote  
 Post subject: Re: constness of *SurfaceExtractor volume argument
PostPosted: Wed Dec 21, 2011 5:10 pm 
Developer
User avatar

Joined: Sun May 11, 2008 4:29 pm
Posts: 198
Location: UK
Generally the pattern I try to follow is that if a function might make a change to an argument then it should be passed in as a pointer and if it will not change it then it should passed as a const reference. Some people recommend the use of references for values which are changed by the function but I feel that using pointers makes it clearer to the user of the API when done our way. This is covered in the Qt style guide (as well as a little in the KDE API policy).

In this particular case, the surface extractor should not be making any change to the volumes and so ideally it should be passed as a const reference but as David says, currently the Sampler is not const-safe so it can't be changed yet. A possible solution would be to have both a Sampler and a ConstSampler (like there are iterators and const_iterators in the STL). Also the considerations of the processing pipeline API must be considered as David says.

I'm keen to have this sort of thing sorted out as I feel const correctness really helps make APIs clearer and easier to use. I'd like to take a few other cues from Qt in future as well such as avoidance of boolean parameters. Personally I consider inconsistent/unclear API as a bug so please don't hesitate to bring things like this up.

_________________
Matt Williams
Linux/CMake guy


Top
Offline Profile  
Reply with quote  
 Post subject: Re: constness of *SurfaceExtractor volume argument
PostPosted: Thu Dec 22, 2011 10:26 am 
Developer
User avatar

Joined: Sun May 04, 2008 6:35 pm
Posts: 1827
milliams wrote:
A possible solution would be to have both a Sampler and a ConstSampler (like there are iterators and const_iterators in the STL).


Yes, except I think the term 'Sampler' already implies it is read only (and they are read only, they are just missing some const keywords). But I have been experimented with allowing them to write as well (e.g. for copying data between volumes) and in this case it starts to make more sense to call the Iterators than Samplers. Maybe we then need ConstIterators or maybe we don't... I'm not sure yet.

milliams wrote:
I'm keen to have this sort of thing sorted out as I feel const correctness really helps make APIs clearer and easier to use.


Yes, and I should also point out that these kind of improvements are under active consideration at the moment, it's just that actual commits are slow due to work on Voxeliens. But changes will be coming.


Top
Offline Profile  
Reply with quote  
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 4 posts ] 

All times are UTC


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
cron
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group
Theme created StylerBB.net