[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
Hi Thorsten, > What I intended to say was, that this feature is not obvious, seldomly > used, and therefore 'obscured' from plain sight - do I make sense now? Yeah, probably. (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.) > And on a related note, as one of our warnings01 heroes, do you have > objections against disabling this warning globally? Nope. Ciao Frank -- - Frank Schönheit, Software Engineer [EMAIL PROTECTED] - - Sun Microsystems http://www.sun.com/staroffice - - OpenOffice.org Database http://dba.openoffice.org - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 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
Frank Schönheit - Sun Microsystems Germany <[EMAIL PROTECTED]> writes: > > I consider this behaviour a useful (if obscure) feature of c++ > > "useful and obscure" ... I'm tempted to call this an oxymoron :) > Depending on your local definition of obscure, of course. ;-) What I intended to say was, that this feature is not obvious, seldomly used, and therefore 'obscured' from plain sight - do I make sense now? And on a related note, as one of our warnings01 heroes, do you have objections against disabling this warning globally? 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
Hi Thorsten, off-topic, but ... > I consider this behaviour a useful (if obscure) feature of c++ "useful and obscure" ... I'm tempted to call this an oxymoron :) Ciao Frank -- - Frank Schönheit, Software Engineer [EMAIL PROTECTED] - - Sun Microsystems http://www.sun.com/staroffice - - OpenOffice.org Database http://dba.openoffice.org - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 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 *,
some more on this - MSVC's C4250 warning, telling us that
"'class1' : inherits 'class2::member' via dominance", prohibits the
following pattern:
struct Interface
{
virtual void a() = 0;
virtual void b() = 0;
};
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();
};
(where the trick is that InterfaceHelper implements the Interface
aspect of DerivedInterface, and DerivedInterfaceImplementation just
the additional methods - methods a() and b(), which are normally pure
virtual again in DerivedInterfaceImplementation due to
DerivedInterface, are overridden by the ones in InterfaceHelper
because of dominance).
I consider this behaviour a useful (if obscure) feature of c++, that
won't cause much harm at other places (especially, since we've
virtually no virtual base classes across OOo - sorry for the pun); did
anyone of the warnings01-Committers found places that triggered this
warning, and there was an actual bug behind it?
I at least never came across that warning, and am +1 for disabling it
globally.
-Stephan
-
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
Hi *,
some more on this - MSVC's C4250 warning, telling us that
"'class1' : inherits 'class2::member' via dominance", prohibits the
following pattern:
struct Interface
{
virtual void a() = 0;
virtual void b() = 0;
};
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();
};
(where the trick is that InterfaceHelper implements the Interface
aspect of DerivedInterface, and DerivedInterfaceImplementation just
the additional methods - methods a() and b(), which are normally pure
virtual again in DerivedInterfaceImplementation due to
DerivedInterface, are overridden by the ones in InterfaceHelper
because of dominance).
I consider this behaviour a useful (if obscure) feature of c++, that
won't cause much harm at other places (especially, since we've
virtually no virtual base classes across OOo - sorry for the pun); did
anyone of the warnings01-Committers found places that triggered this
warning, and there was an actual bug behind it?
Thanks for your feedback,
--
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: Stephan Bergmann <[EMAIL PROTECTED]> writes: Thorsten Behrens wrote: [...] 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? Do we ever want to upgrade the STLport versions we use? I doubt that: On the one hand, we are tied to compatibility issues as STLport details are exposed in the UNO/URE API (though I do not know whether upgrading to later STLport versions would actually be a compatibility issue); on the other hand, if we ever do upgrade to something more modern, I would assume we dump STLport and use the standard libraries that come directly with the compilers. In light of this, neither do I think it would make sense to try to upstream the patches we did to STLport, nor do I see any scalability issues. [...] 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? I agree that if we plan to upgrade boost in the future, and it appears unlikely that future boost versions will no longer trigger C4668, then it might make sense to disable C4668 globally instead of locally within a boost patch. -Stephan - 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::min(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]
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 (C::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::min(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]
[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 (C::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]
