2

This code demonstrates the problem I'm trying to solve:

#include <map>

class Point
{
public:
    float m_x;
    float m_y;
};

typedef std::set<Point *> PointSet;

typedef std::set<const Point * const> ConstPointSet;

float GetMinimumRange(const ConstPointSet &pointSet)
{
    float minimumRange(0.0f);
    // find the smallest distance between any pair of points in the set
    return minimumRange;
}

float GetMinimumRangeWrong(const PointSet &pointSet)
{
    PointSet::iterator first(pointSet.begin());
    Point * point(*first);
    point->m_x = 42.0f;            // I want to prevent this
    return 0.0f;
}

class PointSet_
{
public:
    std::set<Point *> m_pointSet;

    float GetMinumumRange() const
    {
        PointSet::iterator first(m_pointSet.begin());
        Point * point(*first);
        point->m_x = 42.0f;            // I want to prevent this
        return 0.0f;
    }
};

void test()
{
    PointSet myPointSet;
    // Add some points to my set

    // This fails because the compiler states it can't convert from PointSet to ConstPointSet.
    //float minimumRange1(GetMinimumRange(myPointSet));

    // reinterpret_cast<> is the only cast that works here, const_cast fails with the same
    // complaint as the line above generates
    ConstPointSet *myConstPointSet(reinterpret_cast<ConstPointSet *>(&myPointSet));

    float minimumRange1(GetMinimumRange(*myConstPointSet));

    float minimumRange2(GetMinimumRangeWrong(myPointSet));
}

I want to create a routine that takes a PointSet, evaluates the minimum range between any pair of Points in the set, but that it guarantees that it won't modify the PointSet passed to it in any way at all. It can't modify the members of any referenced Point, it can't change the pointers themselves, nor can it add or remove members from the set

The issue is that the compiler correctly views PointSet and ConstPointSet as different types because of the difference of const qualifiers of the inner type, and therefore refuses to cast between them, even though I'm only adding const qualifiers.

I tried creating a class to contain a PointSet, and creating a const member function, but even in there it allows modification to one of the inner Points. At least MSVC will compile that without complaint. I'll confess I was quite surprised about this.

The only way I've found that works is to use a reinterpret_cast<> to convert a pointer to a PointSet to a pointer to a ConstPointSet. The standard does note that reinterpret_cast<> can be used to add const qualifiers, but does that apply in this case?

If not, is there any way to do what I want? I realize that good code discipline can be used to ensure that GetMinimumRange() doesn't modify the passed PointSet, but I'd like to get those const qualifiers in there for two reasons.

  1. They will ensure that if anyone ever modifies GetMinimumRange() they can't cause it to modify the PointSet.

  2. It will allow the compiler to optimize over the call to GetMinimumRange(). In the absence of the const qualifiers, no assumptions can be made at the calling site regarding values that could be cached across the call, thus possibly leading to redundant fetches of data.

dgnuff
  • 3,195
  • 2
  • 18
  • 32
  • 1
    You don't need to mark the pointer const if the container is const. Personally i'd just pass a copy of the container, and not have pointers to the elements. That way it's not physically possible to modify the value the pointers point to, and you can just let the compiler optimize. – BWG Feb 19 '15 at 23:11
  • This causes undefined behaviour by violating strict aliasing. The types being aliased are `set` and `set` which have no relationship unless `T` and `U` are identical – M.M Feb 19 '15 at 23:12
  • @BWG removing the pointers would work, but this is a simplified code sample to demonstrate the poroblem. In practice the `Point *`s are actually pointers to Entites in a game. Using a set<> of these, as opposed to a set of pointers will have some obvious performance problems. – dgnuff Feb 19 '15 at 23:16
  • @MattMcNabb that doesn't solve the problem. The only place `ConstPointSet` exists is in the prototype of `GetMinimumRange()` and it's there for the reasons cited at the bottom of my original post. If I template `GetMinimumRange()`, I might as well remove `ConstPointSet` which then leaves me in the situation of having a routine that wants to take a `PointSet`, but also guarantee that it won't modify it in any way. Given the aliasing issue, it appears that there is no solution to this. – dgnuff Feb 19 '15 at 23:23
  • @dgnuff OK, I see where you're coming from now – M.M Feb 19 '15 at 23:25
  • I'm actually suprised that `PointSet::iterator first(pointSet.begin());` compiles. Shouldn't the `begin()` member function return a const_iterator here? – MikeMB Feb 19 '15 at 23:46
  • note that the error should come on `Point * point(*first);` , not `point->m_x = 42;` if you want an error here – M.M Feb 19 '15 at 23:48
  • @MikeMB Yes, it compiles. At least it does with MSVC. That's exactly the problem I'm trying to solve here, I want to arrange it so that it doesn't. – dgnuff Feb 19 '15 at 23:49
  • @MattMcNabb Indeed. I would like the only legal form of that line to be `const Point * point(*first);` I can certainly write that if I want, but I'm not forced to do so. – dgnuff Feb 19 '15 at 23:52
  • 1
    @MikeMB Elements in a set are constant anyway (to prevent modifying the keys and messing up the search tree), so `set::iterator` and `set::const_iterator` are the same thing. That's why it compiles. – Michael Karcher Feb 20 '15 at 00:24
  • I was specifically wondering about that LoC. You would have the same problem if first was defined as a `const_iterator`. However, then I found out, that std::set::const_iterator and std::set::iterator may actually be the same type, as both point to a const element anyway. To relate to your Question: If the number of elements is small, It would probably be a viable solution to copy the pointers from a `std::set` to a `std::set` before handing it over to the function – MikeMB Feb 20 '15 at 00:25
  • @dgnuff: Btw: Do you actually need a set? Or could you use a different container? – MikeMB Feb 20 '15 at 00:30
  • One more thing: As far as I know, compilers can't optimize based on constness anyway, as it doesn't actually assure, the bits in memory stay unchanged (e.g. due to const_cast or mutable data members). – MikeMB Feb 20 '15 at 00:34
  • @MikeMB I could use just about anything: `std::vector<>`, `std::list<>`, etc. Given that these sets are built one per game session, I could even use a simple `Point **` allocated via `new[]`, I don't need to do a `find()` on the `std::set<>` in the real world, so ordering is unimportant, I just need to iterate over all members. – dgnuff Feb 20 '15 at 00:40
  • @MikeMB re optimization that does remove my reason 2. as valid, but there is still reason 1: I *DO* *NOT* want that `std::set<>` getting changed. – dgnuff Feb 20 '15 at 01:00
  • Even aside from strict aliasing, [class.mfct.non-static]/p2 ("If a non-static member function of a class X is called for an object that is not of type X, or of a type derived from X, the behavior is undefined.") means that you can't legally call any non-static member function on the result of your `static_cast`. – T.C. Feb 20 '15 at 01:05
  • Then you should be able to use @Matt McNabb's solution. Just use a std::vector with his `qualifier_ptr` or make const copies of the pointers. – MikeMB Feb 20 '15 at 01:05
  • `typedef std::set` don't put the second const there. – Neil Kirk Feb 20 '15 at 01:17
  • @BenVoigt the duplicate answers the question of "is it safe" but doesn't address the question of a workaround which OP asked – M.M Feb 26 '15 at 01:03
  • @MattMcNabb: There is *a* workaround provided: use a type which you know not to be specialized. Also, the workaround part is a duplicate of http://stackoverflow.com/q/9142372/103167 But I can't close as dupe of two questions. Such are the problems that arise when a "question" really contains multiple questions. Also this title is beyond useless. So: question which is not searchable because the title is amazingly content-free, does not conform to the one question per question guideline, and does not add anything new to the site. IMO that needs to stay closed. – Ben Voigt Feb 26 '15 at 01:40
  • @BenVoigt Apparently a good many people disagree with you about the usefulness of the title. You personally may not be able to comprehend it, but as can be seen from the numerous replies, several other people made perfectly good sense of it. Also stackoverflow.com/q/9142372/103167 is *NOT* a duplicate of this. That question deals with the constness of the *container*, I'm dealing with the constness of the items pointed to by the pointers within the container. They are very different problems. – dgnuff Feb 28 '15 at 23:00
  • @dgnuff: Your title is useless. I didn't say I can't comprehend it. But it does not serve the purpose of a title, which is to identify the problem you are having. And it IS a duplicate of the other question I marked it as a duplicate of. The essence of both is whether instantiations of standard library types are interchangeable if they vary in const-qualification of the template parameter. The answer to both is that no, instantiations with different types are not interchangeable, not even if the parameter types are layout-compatible with each other. – Ben Voigt Feb 28 '15 at 23:12
  • @BenVoigt Read stackoverflow.com/q/9142372/103167 again. In particular pay close attention to Seth Carnegie's last question and Miro's reply. Miro does not want to change the constness of the pointed to objects. He explicitly states he wants to *maintain* the mutability of the `Object`s that are pointed to by his list<> *which is the exact reverse of what I want* – dgnuff Feb 28 '15 at 23:30
  • @dgnuff: Only because you are defining the problem so narrowly. In both cases, it is desired to have a view of the collection that exposes access with different const-qualification. Whether you are adding or removing `const` is a trivial and obvious modification to the solution. – Ben Voigt Mar 01 '15 at 00:55
  • @BenVoigt That's simply not true. In the general case, taking a `set` and casting it to a `const set` is trivial, and allowed by the spec, just add on the keyword `const`. That's what Miro wants to do. Instead, I want to cast a `set` to a `set` which is explicitly *not* permitted. If one case is explicitly permitted by the spec, and the other is explicitly prohibited, how can they possibly be the same problem? – dgnuff Mar 01 '15 at 02:28
  • @dgnuff: I'm sorry you misunderstood the question I linked to. Miro wanted to return a view that didn't allow container operations (insert, erase, push_back) but did allow overwriting individual elements. Returning a const reference to the container is trivial as you say, but does not enable the required behavior. – Ben Voigt Mar 01 '15 at 02:37
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/71970/discussion-between-dgnuff-and-ben-voigt). – dgnuff Mar 01 '15 at 03:28

3 Answers3

4

There is no straightforward way, because constness does not propagate through pointers. In a const PointSet, it's the pointers themselves that are const, not the objects they point to. And, like you've discovered, const Point * is a different type from Point *, so std::set<const Point *> is a different type from std::set<Point *>.

I don't like the reinterpret_cast of a STL structure. That is scary to me. STL does all kinds of optimizations based on the type of template parameters. std::vector<bool> being an extreme example. You'd think that std::set<T *> and std::set<const T *> would be laid out the same because they are both pointers, but I wouldn't assume so until I read it in the Standard.

If it were a structure I had written myself, and I could easily verify that the cast would work, it would be less scary but still ugly.

You could write a wrapper class that holds a reference to a std::set<Point *> but only allows const access to its pointed-to Points via iterators. If the pointers are guaranteed to be non-null, your iterator can dereference the points directly. I've written it here as a template:

template <typename T>
class PointerSetViewer
{
public:
    PointerSetViewer(std::set<T *> const &set) : set(set) {}

    struct iterator : public std::iterator<std::forward_iterator_tag, T const>
    {
        iterator(typename std::set<T *>::const_iterator it) : it(it) {}
        T const &operator*() const { return **it; }
        T const *operator->() const { return *it; }
        iterator &operator++() { ++it; return *this; }
        bool operator==(iterator other) { return it == other.it; }
        bool operator!=(iterator other) { return it != other.it; }
    private:
        typename std::set<T *>::const_iterator it;
    };

    iterator begin() { return iterator(set.cbegin()); }
    iterator end() { return iterator(set.cend()); }

private:
    std::set<T *> const &set;
};

It's bulky, but it accomplishes your goals without doing anything risky:

float GetMinimumRangeWrong(PointerSetViewer<Point> &pointSet)
{
    PointerSetViewer<Point>::iterator first(pointSet.begin());
    first->m_x = 42.0f;            // does not compile
}

Also if you're using C++11, you can get some nice range-based for loops:

template <typename T>
PointerSetViewer<T> view_set(std::set<T *> const &set) {
    return PointerSetViewer<T>(set);
}

for (Point const &p : view_set(myPointSet)) {
    // whatever...
}

Baroque? Yes, but if one piece of baroque library code lets you write 100 pieces of beautiful application code with better type checking, it's probably worth it.

japreiss
  • 11,111
  • 2
  • 40
  • 77
1

Edit: this doesn't work for set. As pointed out in comments, a non-const set is defined to hold const T, so there is actually nothing we can do.

At this stage I don't see a viable solution other than making PointSet_ actually wrap the set properly, i.e. have the set be private and be careful in your public functions.


Here is a solution I came up with; make the set contain a little wrapper which will propagate the const-ness of itself onto the pointer.

I would have thought there would be a pre-existing class that does this, but none of the std smart pointer classes seem to.

#include <iostream>
#include <set>

template<typename T>
struct qualifier_ptr
{
    T *operator->() { return ptr; }
    T const *operator->() const { return ptr; }

    operator T*() { return ptr; }
    operator T const*() const { return ptr; }

    qualifier_ptr(T *p): ptr(p) {}

private:
    T *ptr;
};

struct Point
{
    float m_x;
    float m_y;
};

struct PointSet
{
    typedef std::set< qualifier_ptr<Point> > SetType;
    SetType points;

    float foo() const
    {
        //Point *p = *points.begin();               // error
        Point const *p = *points.begin();           // OK
        return 0;
    }
};

int main()
{
    PointSet ps;
    PointSet const &cps = ps;
    ps.foo();    // OK
    cps.foo();   // OK
}

I normally don't like to use conversion operators but it seems appropriate here.

M.M
  • 138,810
  • 21
  • 208
  • 365
  • I drafted something like that myself to answer this question, but I rejected it. As `std::set::value_type` is `const T`, there is no difference between a `std::set>` and a `std::set>`. So the line you commented with "error" also wouldn't work in a non-const `foo` function. – Michael Karcher Feb 20 '15 at 00:09
  • @MichaelKarcher argh... so my solution works for `vector` but not `set`. – M.M Feb 20 '15 at 00:17
  • 2
    It would also work for values in a `std::map`. As the OP includes the `` header instead of the `` header, maybe it still is of some use. Your code should also fail in `` because you provide no `operator<(qualifier_ptr,qualifier_ptr)`, no `std::less` instance for `qualifier_ptr` and provide no comparator template argument to `std::set>`. But you might notice that only as soon as you actually call a function on the set that needs to compare elements, like `insert`. – Michael Karcher Feb 20 '15 at 00:21
  • @MichaelKarcher yeah, those things could be added easily enough when it complains. There must be a pre-written wrapper like this somewhere already, where someone has encountered all the issues and fixed them. – M.M Feb 20 '15 at 00:22
  • 2
    There's a proposal for [`propagate_const`](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4372.html). – T.C. Feb 20 '15 at 01:07
  • @TC looks like a very useful thing although unfortunately it still seems to not help with the `std::set` of pointers problem. – M.M Feb 20 '15 at 01:52
0

As you stated in the comments that the set is built only once per session, I'd suggest just creating the ConstPointerSet by making a copy:

void test()
{
    PointSet myPointSet;    
    // Add some points to my set
    ConstPointSet myConstPointSet{ begin(myPointSet), end(myPointSet) };

    float minimumRange1(GetMinimumRange(myConstPointSet));
}

Or wrapp it into a function:

ConstPointSet toConst(const PointSet& pSet){
    return ConstPointSet{ cbegin(pSet), cend(pSet) };
}

If you don't need the semantics of a set I'd recommend using a std::vector instead, which is much more efficient to copy or traverse.

MikeMB
  • 20,029
  • 9
  • 57
  • 102
  • Good point about std::vector<> vs std::set<>. You are correct, using std::set<> is probably a bit of a waste, since it'll sort the pointers themselves, which while harmless is also wasted effort. I'm going to encapsulate a pair of std::vectors<> in an outer PointSet class, and just use whichever is appropriate based on the use case. Since you can't overload on the constness of the return type I'll just have pairs of accessors: PointSet::Foo() and PointSet::ConstFoo(). It'll get the job done. – dgnuff Feb 28 '15 at 23:09