2

I have a doubt about a line of the code written by my professor.

This is the full code.

The relevant function is:

std::vector<AbstractButton*> removeUnchecked() {
    std::vector<AbstractButton*> v;
    CheckBox* p;
    for(auto it = Buttons.begin(); it != Buttons.end(); ++it) {
      p = const_cast<CheckBox*>(dynamic_cast<const CheckBox*>(*it));
      if(p && !(p->isChecked())) {
    v.push_back(p);
    it = Buttons.erase(it); --it;
      }
    }
    return v;
  }

The class Gui has a std::list<const AbstractButton*> Buttons and the function std::vector<AbstractButton*> removeUnchecked(){} wants a vector as its return type. We had to remove from the Gui every checkable buttons with the attribute checked == false and put it into the returned vector.

The professor wrote:

CheckBox* p = const_cast<CheckBox*>(dynamic_cast<const CheckBox*>(*it));

performing a dynamic_cast first, and then a const_cast.

If I had written:

CheckBox* p  = dynamic_cast<CheckBox*>(const_cast<AbstractButton*>(*it))

const_cast first, and then dynamic_cast, would it be the same thing?

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • 1
    Yeah, they are the same. Why? – ALX23z Jan 29 '22 at 08:55
  • 1
    I'd argue the net effect is the same (although language lawyers might be able to pick some nuance that I am not). `const_cast`, given a null pointer, will produce a null pointer. `dynamic_cast` will produce a null pointer if the supplied pointer is either null or does not point at an instance of the class being converted to. `dynamic_cast` can be used to add/maintain `const`ness, but not remove it. So the first produces a pointer (potentially null) which is `const` qualified, then removes removes that `const`ness. The second removes any `const`ness first, then `dynamic_cast`s it. – Peter Jan 29 '22 at 09:03
  • 3
    `const_cast` is a code smell regardless of where it's used. Storing pointers to const in **private** variables when you're storing non-const widgets seems unnecessary. If your class was really big and you wanted the user to explicitly having to express the intent of accessing the non-const version, you'd simply store a wrapper type implementing a conversion operator to `AbstractButton const*` (`operator AbstractButton const*() const`) and `AbstractButton const* operator->() const` in addition to adding a function like `GetAsNonConst`... – fabian Jan 29 '22 at 09:03
  • 1
    Almost forgot: the wrapper type should of course also implement `AbstractButton const& operator*() const` and perhaps `operator bool() const` and comparison operators to `AbstractButton const*`; this adding all these operators make the objects usable as if they were pointers and also allow you do use `wrapperObject.GetAsNonConst()->...` to access the wrapped pointer to non-const... – fabian Jan 29 '22 at 09:14
  • 1
    @fabian - The `operator *()` or `operator bool()` are not needed in this case. The code is working with standard containers of raw pointers (one `const` qualified, and one not). So `*it` will be a pointer, either way, and the casts (in both options shown) are converting raw pointers - there is no wrapper type in play. (I do agree that the `const_cast` is a code smell, but that's not what the question was about). – Peter Jan 29 '22 at 09:26

1 Answers1

1

Both orders of the cast should be fine and do the same thing.

const_cast will return a pointer pointing to the same object as before or a null pointer value if the argument is a null pointer value.

dynamic_cast either returns a pointer pointing to a CheckBox object or a null pointer value, without depending on the const, except that it refuses to cast away a const.


From looking at just the code you linked, it isn't clear to me though why the objects are stored as const pointers in the first place.

const_cast is one of the more dangerous casts. You must guarantee that the pointer which you const_cast doesn't actually point to an object with const type. If it does, then trying to modify the object through the new non-const pointer will cause undefined behavior.


However in the code in your link there is an unrelated bug in the function. It uses std::list::erase to remove the elements while iterating over the list. It correctly uses the return value of std::list::erase as a new iterator to the next element.

But then, because the for loop always executes it++, in the loop body you it-- the return value from std::list::erase to compensate.

However, if you just removed the first element of the list, then std::list::erase will return a pointer to the new first element of the list, i.e. the new .begin() iterator. Decrementing this pointer is not allowed and causes undefined behavior.

user17732522
  • 53,019
  • 2
  • 56
  • 105
  • The `const_cast` is used to remove the constness from the item pointed by the iterator because we have to return a `std::vector`. Are you saying that we could have used the const_cast inside the push_back(here) right? But I've prefered to do everything in a single line. – bandomatteo Jan 29 '22 at 12:26
  • @darkgumma No, I am saying that I don't know why the vector and list private members store `const` pointers. – user17732522 Jan 29 '22 at 12:28
  • I think because the professor wants to be sure if a person knows when to perform a `const_cast `( for example we had a list with `const` pointers and we had to return a vector without the `const`. @user17732522 – bandomatteo Jan 29 '22 at 12:31
  • 1
    @darkgumma OK, but you would normally try to avoid doing such things. They might have chosen it because it is difficult to come up with good examples to use `const_cast`. But still, the important point is that you are not allowed to simply `const_cast` any pointer you want to, You need to make sure that it isn't possible that the pointer refers to an object that is declared `const`. In your example code this seems to be guaranteed in the sense that `insert` takes only non-`const` pointers, so if the class is used correctly it should never be given a pointer to a const object. – user17732522 Jan 29 '22 at 12:49