How to properly use "C++ Core Guidelines: C.146: Use dynamic_cast where class hierarchy navigation is unavoidable"

Question

Motivation

The C++ Core Guidelines recommends using dynamic_cast when "class hierarchy navigation is unavoidable." This triggers clang-tidy to throw the following error: Do not use static_cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-static-cast-downcast].

The guidelines go on to say:

Note:

Like other casts, dynamic_cast is overused. Prefer virtual functions to casting. Prefer static polymorphism to hierarchy navigation where it is possible (no run-time resolution necessary) and reasonably convenient.

I have always just used an enum named Kind nested in my base class, and performed a static_cast based on its kind. Reading C++ Core Guidelines, "...Even so, in our experience such "I know what I'm doing" situations are still a known bug source." suggests that I should not be doing this. Often, I don't have any virtual functions so RTTI is not present to use dynamic_cast (e.g. I will get error: 'Base_discr' is not polymorphic). I can always add a virtual function, but that sounds silly. The guideline also says to benchmark before considering using the discriminant approach that I use with Kind.

Benchmark

enum class Kind : unsigned char {
    A,
    B,
};


class Base_virt {
public:
    Base_virt(Kind p_kind) noexcept : m_kind{p_kind}, m_x{} {}

    [[nodiscard]] inline Kind
    get_kind() const noexcept {
        return m_kind;
    }

    [[nodiscard]] inline int
    get_x() const noexcept {
        return m_x;
    }

    [[nodiscard]] virtual inline int get_y() const noexcept = 0;

private:
    Kind const m_kind;
    int m_x;
};


class A_virt final : public Base_virt {
public:
    A_virt() noexcept : Base_virt{Kind::A}, m_y{} {}

    [[nodiscard]] inline int
    get_y() const noexcept final {
        return m_y;
    }

private:
    int m_y;
};


class B_virt : public Base_virt {
  public:
    B_virt() noexcept : Base_virt{Kind::B}, m_y{} {}

  private:
    int m_y;
};


static void
virt_static_cast(benchmark::State& p_state) noexcept {
    auto const a = A_virt();
    Base_virt const* ptr = &a;

    for (auto _ : p_state) {
        benchmark::DoNotOptimize(static_cast<A_virt const*>(ptr)->get_y());
    }
}
BENCHMARK(virt_static_cast);


static void
virt_static_cast_check(benchmark::State& p_state) noexcept {
    auto const a = A_virt();
    Base_virt const* ptr = &a;

    for (auto _ : p_state) {
    if (ptr->get_kind() == Kind::A) {
        benchmark::DoNotOptimize(static_cast<A_virt const*>(ptr)->get_y());
        } else {
            int temp = 0;
        }       
    }
}
BENCHMARK(virt_static_cast_check);


static void
virt_dynamic_cast_ref(benchmark::State& p_state) {
    auto const a = A_virt();
    Base_virt const& reff = a;

    for (auto _ : p_state) {
        benchmark::DoNotOptimize(dynamic_cast<A_virt const&>(reff).get_y());
    }
}
BENCHMARK(virt_dynamic_cast_ref);


static void
virt_dynamic_cast_ptr(benchmark::State& p_state) noexcept {
    auto const a = A_virt();
    Base_virt const& reff = a;

    for (auto _ : p_state) {
        benchmark::DoNotOptimize(dynamic_cast<A_virt const*>(&reff)->get_y());
    }
}
BENCHMARK(virt_dynamic_cast_ptr);


static void
virt_dynamic_cast_ptr_check(benchmark::State& p_state) noexcept {
    auto const a = A_virt();
    Base_virt const& reff = a;

    for (auto _ : p_state) {
        if (auto ptr = dynamic_cast<A_virt const*>(&reff)) {
            benchmark::DoNotOptimize(ptr->get_y());
        } else {
            int temp = 0;
        }
    }
}
BENCHMARK(virt_dynamic_cast_ptr_check);


class Base_discr {
public:
    Base_discr(Kind p_kind) noexcept : m_kind{p_kind}, m_x{} {}

    [[nodiscard]] inline Kind
    get_kind() const noexcept {
        return m_kind;
    }

    [[nodiscard]] inline int
    get_x() const noexcept {
        return m_x;
    }

private:
    Kind const m_kind;
    int m_x;
};


class A_discr final : public Base_discr {
public:
    A_discr() noexcept : Base_discr{Kind::A}, m_y{} {}

    [[nodiscard]] inline int
    get_y() const noexcept {
        return m_y;
    }

private:
    int m_y;
};


class B_discr : public Base_discr {
public:
    B_discr() noexcept : Base_discr{Kind::B}, m_y{} {}

private:
    int m_y;
};


static void
discr_static_cast(benchmark::State& p_state) noexcept {
    auto const a = A_discr();
    Base_discr const* ptr = &a;

    for (auto _ : p_state) {
        benchmark::DoNotOptimize(static_cast<A_discr const*>(ptr)->get_y());
    }
}
BENCHMARK(discr_static_cast);


static void
discr_static_cast_check(benchmark::State& p_state) noexcept {
    auto const a = A_discr();
    Base_discr const* ptr = &a;

    for (auto _ : p_state) {
        if (ptr->get_kind() == Kind::A) {
            benchmark::DoNotOptimize(static_cast<A_discr const*>(ptr)->get_y());
        } else {
            int temp = 0;
        }
    }
}
BENCHMARK(discr_static_cast_check);

I am new to benchmarking, so I don't really know what I am doing. I took care to make sure that virtual and discriminant versions have the same memory layout and tried my best to prevent optimizations. I went with optimization level O1 since anything higher didn't seem representative. discr stands for discriminated or tagged. virt stands for virtual Here are my results:

Questions

So, my questions are: How should I cast from a base to a derived type when (1) I know the derived type because I checked it before entering the function and (2) when I do not know the derived type yet. Additionally, (3) Should I even be worried about this guideline, or should I disable the warning? Performance matters here, but sometimes it does not. What should I be using?

EDIT:

Using dynamic_cast seems to be the correct answer for downcasting. However, you still need to know what you are downcasting to and have a virtual function. In many cases, you do not know without a discriminate such as kind or tag what the derived class is. (4) In the case where I already have to check what the kind of object I am looking at, should I still be using dynamic_cast? Is this not checking the same thing twice? (5) Is there a reasonable way to do this without a tag?

Example

Consider the class hierarchy:

class Expr {
public:
    enum class Kind : unsigned char {
        Int_lit_expr,
        Neg_expr,
        Add_expr,
        Sub_expr,
    };

    [[nodiscard]] Kind
    get_kind() const noexcept {
        return m_kind;
    }

    [[nodiscard]] bool
    is_unary() const noexcept {
        switch(get_kind()) {
            case Kind::Int_lit_expr:
            case Kind::Neg_expr:
                return true;
            default:
                return false;
        }
    }

    [[nodiscard]] bool
    is_binary() const noexcept {
        switch(get_kind()) {
            case Kind::Add_expr:
            case Kind::Sub_expr:
                return true;
            default:
                return false;
        }
    }

protected:
    explicit Expr(Kind p_kind) noexcept : m_kind{p_kind} {}

private:
    Kind const m_kind;
};


class Unary_expr : public Expr {
public:
    [[nodiscard]] Expr const*
    get_expr() const noexcept {
        return m_expr;
    }

protected:
    Unary_expr(Kind p_kind, Expr const* p_expr) noexcept :
        Expr{p_kind},
        m_expr{p_expr} {}

private:
    Expr const* const m_expr;
};


class Binary_expr : public Expr {
public:
    [[nodiscard]] Expr const*
    get_lhs() const noexcept {
        return m_lhs;
    }

    [[nodiscard]] Expr const*
    get_rhs() const noexcept {
        return m_rhs;
    }

protected:
    Binary_expr(Kind p_kind, Expr const* p_lhs, Expr const* p_rhs) noexcept :
        Expr{p_kind},
        m_lhs{p_lhs},
        m_rhs{p_rhs} {}

private:
    Expr const* const m_lhs;
    Expr const* const m_rhs;
};


class Add_expr : public Binary_expr {
public:
    Add_expr(Expr const* p_lhs, Expr const* p_rhs) noexcept : 
        Binary_expr{Kind::Add_expr, p_lhs, p_rhs} {}
};

Now in main():

int main() {
    auto const add = Add_expr{nullptr, nullptr};
    Expr const* const expr_ptr = &add;

    if (expr_ptr->is_unary()) {
        auto const* const expr = static_cast<Unary_expr const* const>(expr_ptr)->get_expr();
    } else if (expr_ptr->is_binary()) {
        // Here I use a static down cast after checking it is valid
        auto const* const lhs = static_cast<Binary_expr const* const>(expr_ptr)->get_lhs();
    
        // error: cannot 'dynamic_cast' 'expr_ptr' (of type 'const class Expr* const') to type 'const class Binary_expr* const' (source type is not polymorphic)
        // auto const* const rhs = dynamic_cast<Binary_expr const* const>(expr_ptr)->get_lhs();
    }
}

<source>:99:34: warning: do not use static_cast to downcast from a base to a derived class [cppcoreguidelines-pro-type-static-cast-downcast]

        auto const* const expr = static_cast<Unary_expr const* const>(expr_ptr)->get_expr();

                                 ^

Not always will I need to cast to an Add_expr. For example, I could have a function that prints out any Binary_expr. It only need to cast it to Binary_expr to get the lhs and rhs. To get the symbol of the operator (e.g. '-' or '+' ...) it can switch on the kind. I don't see how dynamic_cast will help me here and I also have no virtual functions to use dynamic_cast on.

EDIT 2:

I have posted an answer making get_kind() virtual, this seems to be a good solution in general. However, I am now carrying around 8 bytes for a vtbl_ptr instead of a byte for a tag. Object instantiated from classes derived from Expr will far exceed any other object types. (6) Is this a good time to skip the vtbl_ptr or should I prefer the safety of dynamic_cast?

Answer 1

You might be interested in the Curious Recursing Template Pattern here to avoid the need for virtual method at all if you know the type of the instance at compile time

template <typename Impl> 
class Base_virt {
public:
    Base_virt(Kind p_kind) noexcept : m_kind{p_kind}, m_x{} {}

    [[nodiscard]] inline Kind
    get_kind() const noexcept { return Impl::kind(); }

    [[nodiscard]] inline int
    get_x() const noexcept {
        return m_x;
    }

    [[nodiscard]] inline int get_y() const noexcept { 
        return static_cast<const Impl*>(this)->get_y(); 
    }

private:
    int m_x;
};


class A_virt final : public Base_virt<A_virt> {
public:
    A_virt() noexcept : Base_virt{Kind::A}, m_y{} {}

    [[nodiscard]] inline static Kind kind() { return Kind::A; }

    [[nodiscard]] inline int
    get_y() const noexcept final {
        return m_y;
    }

private:
    int m_y;
};

// Copy/paste/rename for B_virt

In that case, there is no need for dynamic_cast at all since everything is known at compile time. You are loosing the possibility to store a pointer to Base_virt (unless you create a BaseTag base class that Base_virt derives from) The code to call such method must then be template:

template <typename Impl>
static void
crtp_cast_check(benchmark::State& p_state) noexcept {
    auto const a = A_virt();
    Base_virt<Impl> const* ptr = &a;

    for (auto _ : p_state) {
        benchmark::DoNotOptimize(ptr->get_y());
    }
}
BENCHMARK(crtp_static_cast_check<A_virt>);

This is likely be compiled to a strait call to for(auto _ : p_state) b::dno(m_y). The incovenient of this approach is the inflated binary space (you'll have as many instance of the function as there are child types), but it'll be the fastest since the compiler will deduce the type at compile time.

With a BaseTag approach, it'll look like:

   class BaseTag { virtual Kind get_kind() const = 0; }; 
   // No virtual destructor here, since you aren't supposed to manipulate instance via this type

   template <typename Impl>
   class Base_virt : BaseTag { ... same as previous definition ... };

   // Benchmark method become
   void virt_bench(BaseTag & base) {
     // This is the only penalty with a virtual method:
     switch(base.get_kind()) {

       case Kind::A : static_cast<A_virt&>(base).get_y(); break;
       case Kind::B : static_cast<B_virt&>(base).get_y(); break;
       ...etc...
       default: assert(false); break; // At least you'll get a runtime error if you forget to update this table for new Kind
     }
     // In that case, there is 0 advantage not to make get_y() virtual, but
     // if you have plenty of "pseudo-virtual" method, it'll become more 
     // interesting to consult the virtual table only once for get_kind 
     // instead of for each method
   }

   template <typename Class>
   void static_bench(Class & inst) {
     // Lame code:
     inst.get_y();
   }

   A_virt a;
   B_virt b;

   virt_bench(a);
   virt_bench(b);

   // vs
   static_bench(a);
   static_bench(b);

Sorry for pseudo code above, but you'll get the idea.

Notice that mixing dynamic inheritance and static inheritance like the above makes code maintenance a burden (if you add a new type, you'll need to fix all your switch table), so it must be reserved for very small performance sensitive parts of your code.

Answer 2

A possible solution is to make get_kind() a virtual function. You can then use dynamic_cast. If you are going to be calling a lot of virtual functions, you can downcast it to the most derived class so that the optimizer can optimize out the virtual calls. You will also want to use virtual inheritance (e.g. class Unary_expr : public virtual Expr {}; if you do not have any data members in the base class to properly use memory. Having a pointer to the vtable takes up 8 bytes on a 64-bit machine, so you may be forced to use a discriminate to cut down on the size of each object (but this obviously only makes sense if absolutely no virtual functions will be used).

This method solves the following issue raised in the guidlines:

...Even so, in our experience such "I know what I'm doing" situations are still a known bug source.

@xryl669 points out that the "curious recursing template pattern" or CRTP can be used to remove the need of checking the type at runtime if you know what type you are dealing with. He covers a problem and solution of the method too, so you should definatly check his answer out too.

Here is another resource on CRTP I found useful: The cost of dynamic (virtual calls) vs. static (CRTP) dispatch in C++

class Expr {
public:
    enum class Kind : unsigned char {
        Int_lit_expr,
        Neg_expr,
        Add_expr,
        Sub_expr,
    };

    [[nodiscard]] virtual Kind get_kind() const noexcept = 0;

    [[nodiscard]] virtual bool
    is_unary() const noexcept {
        return false;
    }

    [[nodiscard]] virtual bool
    is_binary() const noexcept {
        return false;
    }
};


class Unary_expr : public virtual Expr {
public:
    [[nodiscard]] bool
    is_unary() const noexcept final {
        return true;
    }

    [[nodiscard]] Expr const*
    get_expr() const noexcept {
        return m_expr;
    }

protected:
    explicit Unary_expr(Expr const* p_expr) noexcept : m_expr{p_expr} {}

private:
    Expr const* const m_expr;
};


class Binary_expr : public virtual Expr {
public:
    [[nodiscard]] bool
    is_binary() const noexcept final {
        return true;
    }

    [[nodiscard]] Expr const*
    get_lhs() const noexcept {
        return m_lhs;
    }

    [[nodiscard]] Expr const*
    get_rhs() const noexcept {
        return m_rhs;
    }

protected:
    Binary_expr(Expr const* p_lhs, Expr const* p_rhs) noexcept : m_lhs{p_lhs}, m_rhs{p_rhs} {}

private:
    Expr const* const m_lhs;
    Expr const* const m_rhs;
};


class Add_expr final : public Binary_expr {
public:
    Add_expr(Expr const* p_lhs, Expr const* p_rhs) noexcept : Binary_expr{p_lhs, p_rhs} {}

    [[nodiscard]] Kind get_kind() const noexcept final {
        return Kind::Add_expr;
    }
};


int main() {
    auto const add = Add_expr{nullptr, nullptr};
    Expr const* const expr_ptr = &add;

    if (expr_ptr->is_unary()) {
        // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores): it is just an example
        auto const* const expr = dynamic_cast<Unary_expr const* const>(expr_ptr)->get_expr();

    } else if (expr_ptr->is_binary()) {
        // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores): it is just an example
        auto const* const lhs = dynamic_cast<Binary_expr const* const>(expr_ptr)->get_lhs();
    
        // NOLINTNEXTLINE(clang-analyzer-deadcode.DeadStores): it is just an example
        auto const* const rhs = dynamic_cast<Binary_expr const* const>(expr_ptr)->get_lhs();
    }
}

Answer 3

I think the important part of this guideline is the part about "where class hierarchy navigation is unavoidable". The basic point here being that, if you're wanting to do this kind of casting a lot, then odds are good that there is something wrong with your design. Either you picked the wrong way to do something or you designed yourself into a corner.

Overuse of OOP is one example of such a thing. Let's take your example of Expr, which is a node in an expression tree. And you can ask it questions like whether it is a binary operation, unary operation, or a nullary operation (FYI: literal values are nullary, not unary. They take no arguments).

Where you've overused OOP was in trying to give each operator its own class type. What is the difference between an addition operator and a multiplication operator? Precedence? That's a matter for the grammar; it's irrelevant once you've built the expression tree. The only operation that really cares about the specific binary operator is when you evaluate it. And even when doing evaluation, the only part that's special is when you take the results of the evaluation of the operands and feed it into the code that's going to produce the result of this operation. Everything else is the same for all binary operations.

So you have one function that is different for various binary operations. If there's just one function that changes, you really don't need different types just for that. It's much more reasonable for the different binary operators to be different values within a general BinaryOp class. The same goes for UnaryOp and NullaryOps.

So within this example, there are only 3 possible types for any given node. And that's very reasonable to deal with as a variant<NullaryOp, UnaryOp, BinaryOp>. So an Expr can just contain one of those, with each operand type having zero or more pointers to its child Expr elements. There could be a generic interface on Expr for getting the number of children, iterating through the children, etc. And the different Op types can provide the implementations for these through simple visitors.

Most cases when you start to want to do downcasting and such things are cases that can be solved better and more cleanly using other mechanisms. If you're building hierarchies without virtual functions, where code receiving base classes already knows most or all of the possible derived classes, odds are good that you're really writing a crude form of variant.

Answer 4

All these arguments are wonderful, but there are some cases where these solutions cannot be applied. One example is the seasoned JNI spec. Sony added C++ wrapper to the official Java Native Interface as an afterthought. For example, they define GetObjectField() method that returns jobject. But if the field is an array, you must cast it down to jbyteArray, e.g. to be able to use GetArrayLength().

It is impossible to use dynamic_cast with JNI. The alternatives are either C-style cast, or static_cast, and I believe that the latter is safer, or at least, cleaner than

(jbyteArray)env->CallObjectMethod(myObject, toByteArray_MethodID);

To suppress the warning in Android Studio for a single line, use NOLINT:

auto byteArray = static_cast<jbyteArray>(env->CallObjectMethod(myObject, toByteArray_MethodID)); // NOLINT(cppcoreguidelines-pro-type-static-cast-downcast)

Or, set

#pragma ide diagnostic ignored "cppcoreguidelines-pro-type-static-cast-downcast"

for a file or block