Is this too non-mutable to const?

Question

Basically, I have the following situation. Note: void* is used to denote arbitrary data, it is strongly typed in a real application.

class A
{
public:
   //uses intermediate buffer but doesn't change outward behavior
   //intermediate buffer is expensive to fill
   void foo(void* input_data);

   //uses intermediate buffer and DOES explicitly change something internally
   //intermediate buffer is expensive to fill
   void bar(void* input_data);

   //if input_data hasn't changed since foo, we can totally reuse what happened in foo
   //I cannot check if the data is equal quickly, so I allow the user to pass in the
   //assertion (put the honerous on them, explicitly tell them in the comments
   //that this is dangerous to do, ect)
   void bar(void* input_data, bool reuse_intermediate);
private:
   void* intermediate_buffer_;
   void* something_;
};

So trying for const correctness, intermediate_buffer_ is never exposed so it sortof fits the definition for using a mutable variable. If i never reused this buffer, or I checked for equal input_data before using the cached values, that would be the end of the story, but because of the reuse_intermediate I feel like I'm half exposing it, so I'm not sure whether or not the following makes sense.

class A
{
public:
   //uses intermediate buffer but doesn't change something
   //intermediate buffer is expensive to fill
   void foo(void* input_data) const;

   //uses intermediate buffer and DOES explicitly change something internally
   //intermediate buffer is expensive to fill
   void bar(void* input_data);

   //if input_data hasn't changed since foo, we can totally reuse what happened in foo
   //I cannot check if the data is equal quickly, so I allow the user to pass in the
   //assertion (put the honerous on them, explicitly tell them in the comments
   //that this is dangerous to do, ect)
   void bar(void* input_data, bool reuse_intermediate);

   //an example of where this would save a bunch of computation, though
   //cases outside the class can also happen
   void foobar(void* input_data)
   {
      foo(input_data);
      bar(input_data,true);
   }
private:
   mutable void* intermediate_buffer_;
   void* something_;
};

Thoughts?

Answer 1

I think this is improper use of mutable. In my experience mutable is used for ancillary private member variables that by their very nature can not be declared const, but do not modify the "conceptual constness" of the public interface.

Take for example a Mutex member variable and a 'thread-safe getter':

class Getter { 
public: 

    Getter( int d, Mutex & m ) : guard_( m ), data_( d ) { };

    int get( ) const { Lock l(guard_); return data_; };

private:

    mutable Mutex guard_;
    const int data_;
};

The main point here is that the data declared mutable (in this case the guard) does change (it's locked and unlocked) but this has no impact on constness from the users perspective. Ultimately, despite the mutable Mutex, you still can't change the const data_ member variable and the compiler enforces this.

In your case, you really want the intermediate_buffer to be const but you explicitly tell the compiler it's not by declaring it mutable. The result is that you can change the data and the compiler can't do a thing about it.

See the difference?

If you really want the interface to live up to the const agreement, make it explicit via something like this:

    class A { 
    public:    

        A( void* input_data );// I assume this deep copies.

        void foo() const;

        void bar();

        private:    
            const void* intermediate_buffer_;   
            void* something_; 
    };

Now the onus is truly on the user and enforced by the compiler regardless of what the comments say and without any use of mutable. If they know that input_data has changed, they'll have to create a new one, preferably const.

Answer 2

Instead of hiding the intermediate object inside the const object, expose it and let foo and bar return a copy. You are making it very explicit that the intermediate object is being shared, and providing a new capability to keep more than one on hand. If you want to hide the implementation details you can expose an empty class and make the intermediate object a child of this base class.

class A
{
public:
   class Intermediate
   {
      //empty
   };

   //uses intermediate buffer but doesn't change outward behavior
   //intermediate buffer is expensive to fill
   //if cache_ptr is NULL the intermediate buffer is discarded
   void foo(void* input_data, Intermediate** cache_ptr = NULL) const;

   //uses intermediate buffer and DOES explicitly change something internally
   //intermediate buffer is expensive to fill
   void bar(void* input_data, Intermediate** cache_ptr = NULL);
private:
   class IntermediateImpl : public Intermediate
   {
      //...
   };
   void* something_;
};

Answer 3

To answer your question directly. If the function foo is const, calling it at any time should never alter the result of the next operations.

for example:

A a(some_big_data);
a.bar(some_big_data, true);

should give exactly the same result as (excluding performance differences)

A a(some_big_data);
a.foo(some_different_big_data);
a.bar(some_big_data, true);

Since foo is const, users will expect the result to be the same. If this is the case then making the buffer mutable is reasonable. Otherwise, it is probably wrong.

Hope this helps

Answer 4

Disclaimer: I am not advocating the use of void* with my answer, I hope that this was simply for demonstration purposes and that you don't actually need to use it yourself.

If a lot of computations can be saved when reusing the same input data, then make input_data a member variable.

    class A
    {
      public:
        A(void * input_data)
        {
            set_input_data(input_data);
        }

        //Note, expensive operation
        void set_input_data(void* input_data)
        {
            this->input_data = input_data;
            fill_intermediate_buffer();
        }

        void foo() const;
        void bar() const;
      private:
        void * input_data;
        void * intermediate_buffer;
        void * something;
    };

This is just an outline obviously, without more details about what input_data, intermediate_buffer and something are or how they get used or shared a lot of details will be missing. I would definitely drop the pointers from the implementation and use std::vector. This is especially true of input_data, where you would want to store a copy of the passed-in buffer. Consider:

    A a(input);
    //modifiy input
    a.foo();

You will likely get the wrong result from using a mismatched input_data/intermediate_buffer pair.

Also note that if you don't actually need input_data for foo and bar then you can drop the void* input_data from the class, but still keep the constructor and setter that refer to it.

Answer 5

I'd say that your use of mutable does make some amount of sense - but that it is incorrect, and possibly dangerous. If you make a function const it needs to be just that.

Imagine if you use your class in a multithreaded program, with several threads operating on the same instance of the class - for example:

thread1:

while(1) {
    (...) //do some work
    sharedA->foo(thread1Data);
    sharedA->bar(thread1Data, true);
    (...) //do some more work
    sharedA->bar(thread1Data, true);
    (...) //do even more work
}

thread2:

while(1) {
    (...) //do some different work, maybe wait for an event
    sharedA->foo(thread2Data);
    (...) //sleep for a bit
}

Since thread2 is calling a const function, it shouldn't have any influence at all on the output of calling bar from thread1 - but if the timing is right (or wrong!) it will - and due to those reasons I'd say it's wrong to use mutable to make foo const in this case, even if you're only using the class from a single thread.

Answer 6

Given that you can't check input_data for equivalence, you could perhaps take a cryptographic hash of it and use that for the comparison. This would eliminate the need for the reuse_intermediate flag.

Answer 7

mutable is used to override the const-ness of a variable or data member. Example:

 Class A
  {
    public:
        int m;
  }

  void foo(const A& a);

In the example above, function foo would be allowed to modify a.m even though you pass a const reference. In your example I don't think you need to use mutable - it can lead to very bad designs - since you will write to that buffer.