Enforce const correctness about sgpp HOT 9 OPEN

In GitLab by @valentjn on Nov 17, 2016, 15:00

My two cents:

1. I wouldn't say any part of sgpp::base has stabilized. I'm only involved for 2.5 years, but as far as I can tell, quite a lot of restructuring and refactoring was done, not only since SG++'s beginnings in 2008, but also in those two years. Ok, there is indeed some code which hasn't been touched in years, but not the core things. And the future's no one's to see 😄
2. I generally agree that const correctness is a good thing. In fact, all of my code (i.e., sgpp::optimization) uses it whenever possible, and I already did some efforts to introduce some const correctness in sgpp::base (search the log, examples include sgpp::base::Grid).
3. In an ideal world, you would be completely right. But unfortunately, I think const correctness cannot be enforced all the way in all situations, at least not in a non-intrusive way. Just one example is GridStorage::getSequenceNumber. The GridPoint& index argument cannot be made const in the current implementation: GridStorage::grid_map::find needs a const reference to a GridPoint*, and if we made the index argument constant, the given &index would be a const GridPoint*, which of course is incompatible. I suppose that one would need some serious work to fix that.
4. Besides, const correctness isn't that important in the sense that all non-const-correct code is necessarily bad. My stance is that I use consts whenever I can, but when I can't, due to whatever reason, then I don't.

Are there any specific code areas you thought about for adding const correctness?

from sgpp.

In GitLab by @valentjn on Nov 17, 2016, 15:14

Oh yeah, and w.r.t. the "improve overall code quality without breaking any of the code which builds on top of it": That's only true if you're talking about adding consts to variable declarations in function bodies. If you mean adding consts to argument or return types, it will indeed likely break code building on top of it. Where return types are usually no problem, but argument types are.

That's just the thing about const correctness: If you use it for some code, then you have to use it for all the code which depends on that code. That's why I spent quite some time coding when I was adding some consts in the core of sgpp::base.

from sgpp.

In GitLab by @lettrich on Nov 18, 2016, 18:08

Thank you for you for sharing your experiences and your opinion. I agree with the points you made - especially 4. I opened this "issue" more as a discussion/survey to see how much it matters to the main contributors and how much I should car as ultimately my goal is to write good code and help the project (especially parts of sgpp::datadriven) to improve in features , quality and usability.

As far as adding const correctness, I have not seen too many places in sgpp::base(at least the classes I work with) where I would want to add const but on the other hand many more member functions in sgpp::datadriven that could be revised.

from sgpp.

In GitLab by @MLocs on Dec 14, 2016, 13:44

I think, to move this discussion forward, we need to consider individual parts. And I just happen to have a good example 😉 : the evaluation of the basis functions is symantically stateless and hence should not change the grid structure. I started to add const modifier to the function and then noticed that BsplineClenshawCurtisBasis evaluation actually does. @valentjn: I may be working on the outdated version of the code, is it something that you are using? Is the evaluation is logically const and the user does not see the changes?

from sgpp.

In GitLab by @lettrich on Dec 22, 2016, 14:30

@MLocs I am not entirely sure which member functions you are talking about but the concept sounds logical. Maybe my observation is related to this. I have noticed this for sgpp::base::operationMultipleEval. The eval member function takes the surpluses vector and writes into a result vector. As surpluses should not be changed by the evaluation process, I think the argument should be const. I also think that the eval member function from a conceptual perspective should not change the state of the operationMultipleEval object und thus be a const member function.

from sgpp.

In GitLab by @valentjn on Dec 22, 2016, 14:42

Oops, sorry @MLocs, that one slipped through my attention (blame the many MR mails). No, it's not an outdated version of the code. I agree with both of you, the evaluation should logically be a const method (i.e., user doesn't see the state's changes), but BsplineClenshawCurtisBasis stores some temporary vectors as member variables in the object. Otherwise, for each eval call these objects would've be allocated and de-allocated on the stack, which should be much slower. Downsides of that approach are that you can't use const and that you get issues when trying to parallelize with only one basis object. I guess it's the old discussion "fast/efficient code" vs. "good code" ("good" from a software engineering point of view). I'm not sure if there's an alternative solution that's both fast and good.

from sgpp.

In GitLab by @MLocs on Dec 23, 2016, 13:22

Yes, I addressed both of your concerns (I think) in my current branch as a side note to what I was currently doing 😉
@valentjn The solution was to declare your cache member variables as mutable. This way I tell compiler that those are not seen outside of the class and the methods modifying the cache can still be logically constant.

from sgpp.

In GitLab by @valentjn on Dec 23, 2016, 13:33

Ah--lo and behold! The endless depths of the many mysteries of C++ provide a solution for this very problem (usually the depths introduce even more mysteries). I did not know the mutable keyword. Guess it's never too late to learn new things 😄

Thanks and Merry Christmas Valeriy (I just saw I forgot it in the e-mail).

from sgpp.

In GitLab by @MLocs on Dec 23, 2016, 13:40

Merry Christmas to you too! 🎅 :mother_christmas: 🎄 🎁

from sgpp.