mapbox / variant

C++11/C++14 Variant
BSD 3-Clause "New" or "Revised" License
371 stars 101 forks source link

warning: 'bad_variant_access' #95

Closed springmeyer closed 8 years ago

springmeyer commented 8 years ago

With clang++ -Weverything I see:

deps/mapbox/variant/variant.hpp:58:7: warning: 'bad_variant_access' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Wweak-vtables]
class bad_variant_access : public std::runtime_error
      ^

/cc @artemp - worth fixing?

joto commented 8 years ago

I think this is not fixable if I understand the error correctly. We want to derive our exception class from std::runtime_error which is a virtual class. But this being a header-only library there is no way to define the methods of our class anywhere but in the header. There is no .cpp file to define them in.

artemp commented 8 years ago

Yes, as long as we want to derive from std::runtime_error or std::exception we'll face this issue.

Having a look at implementation - with our current usage there's no benefit or requirement to derive from std::runtime_error and it'll be better to derive from std::exception (same as boost::variant)

class bad_variant_access : public std::exception
{
public:
    virtual const char* what() const noexcept
    {
        return "mapbox::util::bad_variant_access: FAIL";
    }
}; // class bad_variant_access

At least the above is slightly more efficient ^

/cc @joto @springmeyer

joto commented 8 years ago

See #48. I specifically used std::runtime_error to be backwards compatible with current user code that might check for it, but I believe this should be std::logic_error.

As for your proposed change: I don't think making exceptions more efficient is really important, but otherwise that change looks okay to me if we are sure we always want a fixed string as the message here.

springmeyer commented 8 years ago

Okay thanks for the details and discussion. Going to close as I agree there is nothing needing fixed.

ansarikhurshid786 commented 4 years ago

I m also facing same issue.

anyone have solution or workaround?