[dev] Is override via dominance useful (was: [dev] Warning free code: some more warnings that might be disabled)
Frank Schönheit - Sun Microsystems Germany [EMAIL PROTECTED] writes:
(What I intended to say was: what I've learned during reading zillions
of line of code inherited from others is that patterns which are
obscured from plain sight are hard to maintain, and thus prone to
introducing errors. While using cool language features is, well, cool,
its usefulness sometimes decreases over time, when it comes to
maintaining the code. KISS. But hey, I'm digressing.)
Generally, true. But aren't we c++ programmers accustomed to an
unprecedented level of obscureness, anyway (at least compared to, say,
a Java coder)? ;-)
So, in this case, about the only change that will break things with
this idiom (and doesn't trigger a compiler error) is that someone
implements method a() or b() in Interface:
struct Interface
{
- virtual void a() = 0;
+ virtual void a() {}
- virtual void b() = 0;
+ virtual void b() {}
};
struct DerivedInterface : public Interface
{
virtual void c() = 0;
};
struct InterfaceHelper : public virtual Interface
{
virtual void a();
virtual void b();
};
struct DerivedInterfaceImplementation :
public virtual DerivedInterface,
protected InterfaceHelper
{
virtual c();
};
and expects that those are called in DerivedInterfaceImplementation,
instead of the overrides from InterfaceHelper.
Admittedly, this language feature _is_ dangerous when using multiple,
virtual inheritance with concrete classes (instead of interfaces, as
in my idiom), because then, figuring out by hand which method wins can
get quite involved. But I don't think we do that now, and we certainly
shouldn't do that for new code (since there are far more pitfalls with
that than this one).
Cheers,
--
Thorsten
If you're not failing some of the time, you're not trying hard enough.
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Warning free code: some more warnings that might be disabled
Thorsten Behrens wrote: Hi folks, while trying to get basebmp and vigra warning free, I've been bitten by the following two msvc warnings: - Compiler Warning (level 3) C4686 'user-defined type' : possible change in behavior, change in UDT return calling convention A class template specialization was not is defined before it was used in a return type. Anything that instantiates the class will resolve C4686; declaring an instance or accessing a member (Cint::anything) are also options. - Compiler Warning (level 4) C4668 Error Message 'symbol' is not defined as a preprocessor macro, replacing with '0' for 'directives' A symbol that was not defined was used with a preprocessor directive. The symbol will evaluate to false. To define a symbol, you can use either the #define directive or /D compiler option. - C4686 is one of those infamous 'let's warn our users, because we've changed behaviour and fixed a nonconformant behaviour' MS noise generators (it's about which calling convention is used for the not-yet-instantiated return type). I'd say could safely be disabled. Agreed that such warnings should be disabled globally. (I also want to disable warning C4347: behavior change: 'overload A' is called instead of 'overload B' on CWS sb59, which is for example triggered by some instances of std::minsal_Int32(a,b).) C4668 can potentially unearth real portability problems (that a try to access an undefined preprocessor macro evaluates to '0' is a convention possibly not shared by all c++ compilers of this universe) This behavior is not a convention, but mandated by the C/C++ standards. Still, in all the cases I encountered so far where MSC issued this warning, it either (a) unearthed really broken code like #if STRCODE==sal_Unicode in tools/source/string/strimp.cxx:1.7, (b) was easy to either fix the offending #if FOO to something like #ifdef FOO (which appeared to be the intended code, and the original author had merely been sloppy), or (c) was easy to wrap the offending (external) header in some disable-warnings #pragma (which was needed anyway, as the external header also produced other warnings). Thus, I am reluctant to disable this warning globally. - unfortunately, it's implicitely relied upon by boost's BOOST_WORKAROUND macro (boost/workaround.hpp). Since no other compiler warns about that, I've no idea how to fix BOOST_WORKAROUND, and this macro's use is quite abundant in boost, I'd also suggest disabling it. This could be temporary, until upstream boost has fixed/workarounded this. Would it work to disable that warning locally within boost headers? Last, but not least there's a really annoying side effect of gcc's -Wshadow warning: when declaring functions that overload one of the gcc builtin functions, you get the following warning issued: stl/stl/_complex.h:782: warning: shadowing built-in function `int std::abs(int)' Switching off -Wshadow cures this, as well as disabling gcc's builtins via -fno-builtin. But both prolly aren't changes we want to do persistently for gcc. Any other ideas? We already sprinkled our STLport patches with pragmas to disable warnings locally within those headers. Seems this is just yet another place where we need to do that. -Stephan Thanks for listening, feedback greatly appreciated, - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Warning free code: some more warnings that might be disabled
Stephan Bergmann [EMAIL PROTECTED] writes: Thorsten Behrens wrote: - Compiler Warning (level 3) C4686 'user-defined type' : possible change in behavior, change in UDT return calling convention [...] Agreed that such warnings should be disabled globally. (I also want to disable warning C4347: behavior change: 'overload A' is called instead of 'overload B' on CWS sb59, which is for example triggered by some instances of std::minsal_Int32(a,b).) Ok - will disable it then. C4668 can potentially unearth real portability problems (that a try to access an undefined preprocessor macro evaluates to '0' is a convention possibly not shared by all c++ compilers of this universe) This behavior is not a convention, but mandated by the C/C++ standards. Still, in all the cases I encountered so far where MSC issued this warning, it either (a) unearthed really broken code like #if STRCODE==sal_Unicode in tools/source/string/strimp.cxx:1.7, (b) was easy to either fix the offending #if FOO to something like #ifdef FOO (which appeared to be the intended code, and the original author had merely been sloppy), or (c) was easy to wrap the offending (external) header in some disable-warnings #pragma (which was needed anyway, as the external header also produced other warnings). Thus, I am reluctant to disable this warning globally. - unfortunately, it's implicitely relied upon by boost's BOOST_WORKAROUND macro (boost/workaround.hpp). Since no other compiler warns about that, I've no idea how to fix BOOST_WORKAROUND, and this macro's use is quite abundant in boost, I'd also suggest disabling it. This could be temporary, until upstream boost has fixed/workarounded this. Would it work to disable that warning locally within boost headers? In principle, yes - only that there are currently ~25 files to clobber with this (but see below). Last, but not least there's a really annoying side effect of gcc's -Wshadow warning: when declaring functions that overload one of the gcc builtin functions, you get the following warning issued: stl/stl/_complex.h:782: warning: shadowing built-in function `int std::abs(int)' Switching off -Wshadow cures this, as well as disabling gcc's builtins via -fno-builtin. But both prolly aren't changes we want to do persistently for gcc. Any other ideas? We already sprinkled our STLport patches with pragmas to disable warnings locally within those headers. Seems this is just yet another place where we need to do that. Hm. I'm not sure whether this is an approach that scales. Admittedly, the ~180 cases in STLport-4.0.patch are generated automatically - but is anything of that going to be upstreamed? There's of course a fine line between fixing obviously incorrect code, and mutilating it to the likings of specific compiler's warning sets. The latter does not make the code more portable, and significantly increases the chance of build breakage on the platforms not built (unless there are zero-cost mechanisms to build on _all_ werror=t platforms - c.f. build bots). I'm not suggesting that C4668 falls into this category (-Wshadow clearly does not), but I'm convinced that we must agree on a set of warnings that's enforced (werror=t), and a superset that can optionally be used by the developer to improve code quality. For the former, we should err on the safe side, the latter could also contain more controversial stuff (I'm still a big fan of gcc's -Weffc++). Would you agree that having a warning enabled that has uncovered one real bug in N years of development, offers a bad cost/benefit ratio if it needs recurring fixes in external code/frequently breaks HEAD? Cheers, -- Thorsten If you're not failing some of the time, you're not trying hard enough. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
[dev] Warning free code: some more warnings that might be disabled
Hi folks, while trying to get basebmp and vigra warning free, I've been bitten by the following two msvc warnings: - Compiler Warning (level 3) C4686 'user-defined type' : possible change in behavior, change in UDT return calling convention A class template specialization was not is defined before it was used in a return type. Anything that instantiates the class will resolve C4686; declaring an instance or accessing a member (Cint::anything) are also options. - Compiler Warning (level 4) C4668 Error Message 'symbol' is not defined as a preprocessor macro, replacing with '0' for 'directives' A symbol that was not defined was used with a preprocessor directive. The symbol will evaluate to false. To define a symbol, you can use either the #define directive or /D compiler option. - C4686 is one of those infamous 'let's warn our users, because we've changed behaviour and fixed a nonconformant behaviour' MS noise generators (it's about which calling convention is used for the not-yet-instantiated return type). I'd say could safely be disabled. C4668 can potentially unearth real portability problems (that a try to access an undefined preprocessor macro evaluates to '0' is a convention possibly not shared by all c++ compilers of this universe) - unfortunately, it's implicitely relied upon by boost's BOOST_WORKAROUND macro (boost/workaround.hpp). Since no other compiler warns about that, I've no idea how to fix BOOST_WORKAROUND, and this macro's use is quite abundant in boost, I'd also suggest disabling it. This could be temporary, until upstream boost has fixed/workarounded this. Last, but not least there's a really annoying side effect of gcc's -Wshadow warning: when declaring functions that overload one of the gcc builtin functions, you get the following warning issued: stl/stl/_complex.h:782: warning: shadowing built-in function `int std::abs(int)' Switching off -Wshadow cures this, as well as disabling gcc's builtins via -fno-builtin. But both prolly aren't changes we want to do persistently for gcc. Any other ideas? Thanks for listening, feedback greatly appreciated, -- Thorsten If you're not failing some of the time, you're not trying hard enough. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
