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?

Menard

for the first question: If you checked before the function and you know it is of derived type, then why does the function take a reference to base? – Theotokos

"I am new to benchmarking, so I don't really know what I am doing. I ... tried my best to prevent optimizations.". C++ without optimizations is used to debug code, but the resulting performance is meaningless. That's generally no big problem as the human debugging is the slowest component. – Lighting

If you don't have virtual and you static_cast from parent to child, I would be suspicious. Looks like composition would be better and the cast is also avoidable. If you know the type before entering the function, cast it before entering the function. if you don't know the derived type yet, then RTTI (with virtual...)/dynamic_cast is a solution, you can also use static_cast as it's faster. That's where you have to draw the line. – Toscano

@MatthieuBrucher, So I have been casting before entering the function, but that just moves the problem to what cast to use before entering the function. – Menard

@MSalters, If I use higher optimizations, it optimizes the program away. The compiler simply has to much information for this example. What if I am casting from a Expr to Binary_expr or Add_expr. No virtual functions at in the hierarchy. – Menard

@user1032677: So, what you discovered is that in real-world environments the operations can take 0.0 seconds. That shows how well the optimizer understands what dynamic_cast does. – Lighting

@Lighting I mean the object was declared on the previous line. It's not going to make that optimization if it came from the free store where it can't know which function created the object, which is more likely in my case – Menard

Well, then benchmark what you are trying to benchmark. That particular optimization is real and useful. For users that follow the guideline which started the question, it means they don't have a runtime penalty when the compiler can avoid it. – Lighting

@Lighting You were right: youtu.be/ARYP83yNAWk?t=206 – Menard

@MatthieuBrucher @Lighting I edited the question with and what I am truly trying to do. I have no virtual functions, yet I want to downcast. Any thoughts? – Menard

My view is, if you don't have a virtual but want to downcast, then you have a design problem. Maybe there is a complex reason why you want to do this, but if you have to ask this question, it means you don't have the required skill to do it and thus you shouldn't use this design. Composition seems to be more adequate. – Toscano

@Matthieu Brucher, What about in my Expr example where the vtbl_ptr takes up 50% of the objects memory layout for a Unary_expr? Classes deriving from Expr (or similar such as Type) will be the most instantiated objects. CRTP seems to solve this, but I will still need to be able to use a pointer to the base case. @xryl669 answer suggests using a virtual function to tag it, but that kinda ruins the point (or am I missing something?). Since all of the objects I allocate will be only several bytes, I am not certain the vptr is a good trade off even if its fast. – Menard

@user1032677: "What about in my Expr example where the vtbl_ptr takes up 50% of the objects memory layout for a Unary_expr?" Why is Expr a base class rather than some form of variant type, where the possibilities are clearly specified? This is probably what Matthieu was talking about with "a design problem": that there are better ways to do the thing you're trying to do than using inheritance. Your Expr class could easily be a std::variant<BinaryOp, Literal>, where BinaryOp is a struct that contains two other sub-expressions and a value telling which operation it performs. – Collegium

@NicolBolas, I see what you are saying, and that is very useful. However, in my case I have about 40 different kinds. gtoe_expr, modulo_assignment_expr, double_lit_expr. Am I really expected to pack this all into a std::variant? – Menard

@user1032677: There's a reason why I ignored your several types and broke them down into two: a binary operator (which represents any binary operator, with a particular instance storing the specific type of operator) and a literal terminal value. You don't need a separate type for every kind of binary operator. You are overusing OOP, which is typically what has happened when you start wanting to downcast like that. – Collegium

@NicolBolas, So I will still use a tag (one enum class for Unary_expr, another for Binary_expr, ...). That's an acceptable answer. Thanks! – Menard

@NicolBolas Yes indeed, thanks for explaining better! @user#032677 I think you need to practice a little bit more, take some steps back, experiment with different other problems before coming back to this one and enhance the design properly. – Toscano

Answer 1

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.

Collegium

So in the most basic form Expr* points to a std::variant? – Menard

A potential design flaw with this method is that you are repeating yourself. The variant stores information for the arity of the expression, but each kind of expression family will need a kind tag to tell you if you are looking at an addition or subtraction expression. If my kind tag says it is addition then I know it's a Binary_expr. Doesn't this go against the DRY principal and use more memory than necessary? – Menard

@user1032677: I don't see how this relates to DRY. The questions "is this a binary operation" and "what kind of binary operation is it" are two separate questions, asked by two separate places in code, which have two separate responses, leading to two separate uses. The question of memory consumption is too trivial to bother looking into; even if you have an expression tree with a 100,000 nodes, you're talking about less than 512KB of "more memory". – Collegium

By not inheriting from Expr, am I not misusing the "Is-A" relationship? – Menard

Answer 2

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.

Deputize

The BaseTag approach seems equivalent to my Kind approach except that get_kind() is now virtual. After learning about the downcast optimization of dynamic_cast, it would appear that it will always be as good as checking a tag and then static_casting based on that. – Menard

Yes, but if you are chaining your code with a succession of dynamic_cast (if dc<A> else if dc<B> else if dc<C> etc..) then you'll pay the virtual test X times (since you can't switch on the type in C++), while with the BaseTag & Kind approad, you can. – Deputize

Answer 3

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();
    }
}

Menard

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

Bruin