3

I've been working for longer now with the 3ds max SDK, which in nearly all parts doesn't use const at all. So even a Width() or Height() getter of a Bitmap isn't marked as const. This has been already a real annoyance in small projects, but since I've been working on a larger project, it becomes increasingly terrible.

For example, I am holding a single Bitmap instance as a shared_ptr<Bitmap> member in multiple class instances out of performance reasons. Of course there are cases I want to avoid by all means that a single instance may change the properties for all instances, thus all raw pointer getters (necessary for the SDK) deliver a const Bitmap*. Unfortunately, now I can't even ask the const Bitmap* for its width - because Width() is non-const.

I'm asking myself what is the best way to deal with this. I see three options:

  • Forget about const completely, make everything non-const. In the smaller projects I used to do this, but like I said, with more sophisticated techniques, it becomes more dangerous.
  • Make an inplace const_cast at every place it's necessary. That will be at many places, and it's pretty bad to read.
  • Write wrappers for the 3ds max classes, which provide const methods at least for the methods which are highly presumably safe. This would encapsulate all the const_cast in one place and be also suitable for other projects.

I have been warned (and I know) that this might be opinion-based. But I had to deal with this annoying problem for a long time now, and I'd really like to find a solution and thus need the experience of others.

user2328447
  • 1,807
  • 1
  • 21
  • 27
  • Fourth option: ditch the library. From what you mention, it doesn't inspire confidence. – juanchopanza Feb 17 '18 at 10:25
  • Haha, true, this is a preferrable option :) But this unfortunately would mean I'd have to ditch a part of my job as well... – user2328447 Feb 17 '18 at 10:27
  • I'd see two solutions here : wrap every single thing in a const-aware wrapper (your 3rd option) / just like @juanchopanza suggested, ditch the library – Vivick Feb 17 '18 at 10:30
  • I think the 3rd option is better in term of modern functional programming, where all are constants and using variables is the exception. – user9335240 Feb 17 '18 at 10:30
  • 2
    One problem to consider with wrapping the `API` is that you add a maintenance burden. If their `API` changes you have to mirror those changes in your wrappers. – Galik Feb 17 '18 at 10:41
  • 1
    I wouldn't overthink it and go with number 1; const correctness is a bit like a clean history (rebase instead of merge) in git: nice in theory, people pour a lot of time in it, but ultimately way overrated in its usefulness. Many other mainstream languages don't have it and they do just fine. – Matteo Italia Feb 17 '18 at 10:42
  • 2
    @Galik I strongly disagree with your statement, API wrappers tend to reduce maintenance burden as such approach is capable of enclosing modifications caused by API changes inside of wrapper itself often eliminating need for modifications in the code using the wrapper. That is one can often get away with changing only wrappers instead of changing entire code base. – user7860670 Feb 17 '18 at 10:46
  • @MatteoItalia Sounds like pretty bad advice. In my experience, the lack of locality that languages without const have puts too much of a burden on the users of the code to understand what is going on. The only times languages w/o const are "fine" in my experience, is when things cannot get mutated anyway (Im thinking of functional programming, but an imaginary procedural language with only value semantics would also probably be fine.) – juanchopanza Feb 17 '18 at 12:42
  • 2
    @user2328447 • you've discovered -- painfully -- the viral nature of const correctness. I'd write const correct proxy wrappers as a firewall to the const-free Library/SDK. – Eljay Feb 17 '18 at 15:19
  • 2
    I effectively forked a lib for this reason once. It was dreadful in these terms but otherwise exactly what I needed. – Lightness Races in Orbit Feb 17 '18 at 15:38
  • Thank you all for your answers, comments and insight. I have now decided to go with VTT's wrappers approach. – user2328447 Feb 20 '18 at 23:13

2 Answers2

3

First of all, I would like to mention that lack of const correctness may be justifiable by implementation details, for example getter function may perform lock on internal synchronization primitive and therefore always alters internal state and can not be marked as const:

int Bitmap::Width(void)
{
     int width{};
     ::std::lock_guard<::std::mutex> const lock{m_sync};
     width = m_width;
     return width;
}

As a workaround you can write a dedicated PImpl bitmap wrapper restricting direct access to bitmap implementation forwarding functions of interest with appropriate const qualifiers:

class SharedBitmap
{
    private: ::std::shared_ptr<Bitmap> m_p_bitmap;

    public: int Width(void) const
    {
        return m_p_bitmap->Width();
    }

    // other methods...
};

Note that this approach is different from third option listed in question as it does not involve const_cast.

user7860670
  • 35,849
  • 4
  • 58
  • 84
  • 3
    That's a good idea - I have to think about it. But regarding the mutexes: I usually declare especially mutexes `mutable`, because in my understanding they are not really a part/member of the class, but a special item to lock the "real" members against threading conflicts. Thus also getters, which have to lock can be const. – user2328447 Feb 17 '18 at 10:58
  • @James And how CRTP could be applied here? Also note that OP is already using a `shared_ptr` so wrapping it into PImpl class seems like a logical next step. – user7860670 Feb 17 '18 at 11:38
  • @user2328447 I think it is worth making *absolutely sure* that the non-const functions could really be `const` before treating them as such. Maybe somewhere deep in the call stack some unsynchromized global gets modified, for example. – juanchopanza Feb 17 '18 at 12:50
  • 1
    _"for example getter function may perform lock on internal synchronization primitive and therefore always alters internal state and can not be marked as const:"_ That's nonsense - this is what `mutable` is for. – Lightness Races in Orbit Feb 17 '18 at 15:38
  • @juanchopanza true. Unfortunately I haven't got the complete source, but at least a lot of getters should be safe, since I can trace in the headers where they read the members. – user2328447 Feb 20 '18 at 23:15
1

In my experience (10yr), "const" has been a greater nusiance than it has been helpful. Not to mentions code getting longer, ergo harder to read. If you want to know how a library works, you read the manual anyway, not the header. If you want to know you did it right, you run functional tests. Hell there are even static analysis tools checking if a variable is ever written to, without burdening the code with useless non-functional metadata to capture undocumented usage patterns. And since there are a many ways to break const, it is the right way of capturing such errors.

In summary, option 1 is in my experience the most efficient solution. (Is this an opinion? Those who disagree will probably think so.)

For that quick post-purge of const you could do #define const or even -Dconst to remove it, though whether it is safe may depend on your specific scenario, one illegal use is doing this for the standard headers. I have done similar hacks like #define private|protected public instead of messing with friend when doing white box testing, works like a charm!

Know that the concept of "constant variable" is void in many programming languages and they seem to do just fine without it.

The only time you need const is in the case of C-string constants / string literals. Does not seem to be your case though.

Andreas
  • 5,086
  • 3
  • 16
  • 36
  • const gives you a pretty good indicator as to the thread-safetyness of a function – James Feb 17 '18 at 11:37
  • @James `const` does not give any hints about thread-safetyness of a functions because C++ only provides a weaker kind of const-correctness. That is const-qualified function can still access internal state of the object or some static values that can be altered by some other code. – user7860670 Feb 17 '18 at 11:46
  • Are you aware that redefining C++ keywords can easily introduce undefined behaviour? If you use the standard library in your code, then all bets are off. – juanchopanza Feb 17 '18 at 12:33
  • @VTT accessing state is read-only which is thread safe, changing it should be atomic or mutex protected, both of which should be mutable, and therefore the function can be const. As for static values... probably best to not do it, or do it correctly. Sure, it's easy to outsmart the compiler but you get what you deserve if you do that. – James Feb 17 '18 at 12:37
  • 1
    @James There is no need to outsmart anything. There is simply no connection between presence of const qualifier and thread safeness of the function. It is completely wrong to make any assumptions about changes of internal state being "atomic or mutex protected" based on presence of const qualifier. – user7860670 Feb 17 '18 at 12:58
  • @juanchopanza In case of arbitrary keywords, yes of course. In case of the ones I mention (const|private|protected), not really. Might run into duplicate definitions in the case of silencing const, but that just simply won't compile; not UB. If you have something particular in mind please let me know and I'll remove any faulty suggestion of solution. – Andreas Feb 17 '18 at 13:32
  • @Andreas It is what is specified in the standard. There's no distinction based on which keyword. – juanchopanza Feb 17 '18 at 13:42
  • @juanchopanza: _"Are you aware that redefining C++ keywords can easily introduce undefined behaviour"_ So easily that you don't need to do anything else. Your program literally has undefined behaviour the moment you've redefined a C++ keyword. There is nothing to introduce. – Lightness Races in Orbit Feb 17 '18 at 15:40
  • @Andreas: _"In case of the ones I mention (const|private|protected), not really"_ Yes really. – Lightness Races in Orbit Feb 17 '18 at 15:40
  • @LightnessRacesinOrbit `#define const` essentially removes any occurrence of the token `const` from the file during pre-processing. During this phase `const` has no meaning. After that there are no occurances of `const` during compile. How can it be UB if the token does not exist during neither pre-processing, compile or link? It is as it was never written in the first place. Show proof or cut the crap. – Andreas Feb 17 '18 at 17:04
  • 2
    @Andreas: I don't think you understand what UB is. The standard says that if you do this, the behaviour of your program is undefined. Period. End of story. Perform [basic research](https://stackoverflow.com/q/9109377/560648) before telling people to "cut the crap" – Lightness Races in Orbit Feb 17 '18 at 17:12
  • 1
    Wow, speaking of crap, I hadn't yet read your answer until now. Dreadful advice all around. You are, of course, entitled to your own opinion; however, your opinion is wrong. :) – Lightness Races in Orbit Feb 17 '18 at 17:18
  • @LightnessRacesinOrbit maybe you should do some research yourself. in your links there are quotes from the standard (i.e. "proof") saying it is illegal only for the standard libraries. talk about shooting yourself in the foot. – Andreas Feb 18 '18 at 09:19
  • @Andreas If you *use* the standard library (directly or indirectly), then you have UB. BTW that's why I used weasel words originally, "*can easily introduce undefined behaviour*" instead of something stronger. – juanchopanza Feb 18 '18 at 09:32
  • 1
    @Andreas: No, it is illegal in your own code (unless you use no standard headers, but I'd love to see that). Cut it out with the personal attacks and listen/learn. – Lightness Races in Orbit Feb 18 '18 at 14:14