Re: [Development] Branch for adding Q_DECL_OVERRIDEs

[Thiago Macieira]

On quinta-feira, 27 de outubro de 2016 16:43:59 PDT Александр Волков wrote:
> Hi all,
> 
> Many methods are still missing 'override' specifier.
> You can find them by compiling with gcc's -Wsuggest-override.
> It improves code readability and helps to find unused code
> (e.g. https://codereview.qt-project.org/#/c/174370/ )
> Are there plans about adding 'override' or 'Q_DECL_OVERRIDE'
> and which branch is preferred for such changes?

I would prefer we did not add Q_DECL_OVERRIDE at all to current code. We may 
add "override" if that keyword is allowed by all compilers, otherwise leave it 
as it is.

GCC and Clang support a warning for "inconsistent override", which is caused 
by having some overridden methods marked but not others. So the recommendation 
is:

1) for new classes, always mark with Q_DECL_OVERRIDE

2) for existing classes, if possible use Q_DECL_OVERRIDE, but be careful to be 
consistent (be careful with #ifdef'ed functions too!). It's ok to add a 
handful of other Q_DECL_OVERRIDE to other functions in the same class, but 
don't do it if it uglifies your patch. Instead, leave it out.

3) if you get an inconsistent override use warning, fix it

Note that only #3 applies to the 5.6 branch. You can't add new public classes 
or public methods in 5.6 or 5.7. And you should not be adding even new, 
private, polymorphic classes in those branch. And there's only a short window 
while that is permitted in 5.8 too.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Branch for adding Q_DECL_OVERRIDEs

[Marco Bubke]

Final can be used by the compiler by optimazing the virtual call away but it 
has the drawback that you cannot overload it in testing code to mock or fake a 
class. 


On October 27, 2016 18:36:01 Giuseppe D'Angelo  
wrote:

> Hi,
>
> please keep the discussion on the mailing list, so others can chime in.
>
> Il 27/10/2016 17:47, Александр Волков ha scritto:
>> 27.10.2016 18:19, Giuseppe D'Angelo пишет:
>>> Don't worry too much. To minimize merge conflicts I'm quite sure we'll
>>> keep using Q_DECL_OVERRIDE as long as 5.6 will be open. So use that :)
>>> We'll do a cleanup pass in the future.
>> 
>> BTW, should 'override' be added in the following case:
>> struct A {
>>virtual void f();
>> };
>> struct B : A {
>>void f() final; // override also?
>> };
>> ?
>> 
>> Unfortunately gcc reports a warning for f() 
>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78010)
>> There some such places in Qt, for example in 
>> corelib/kernel/qeventdispatcher_glib_p.h,
>> where Q_DECL_OVERRIDE should be added to make it compilable with gcc 
>> [-Werror, -Wsuggest-override].
>> The question is whether to follow a reasonable style
>> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rh-override
>> or a style forced by gcc?
>
> To be honest I don't know if we have a consensus on this. (How many
> cases of "final" in our classes do we have anyhow?)
>
> Cheers,
> -- 
> Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer
> KDAB (UK) Ltd., a KDAB Group company | Tel: UK +44-1625-809908
> KDAB - Qt, C++ and OpenGL Experts
>
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Branch for adding Q_DECL_OVERRIDEs

[Konstantin Tokarev]

27.10.2016, 19:44, "Rolland Dudemaine" <roll...@ghs.com>:
> Using the GHS compiler, i see thousands of examples of this.
> To the point that (bad practice warning!) I disabled the warning.

Better take clang-tidy and fix up things automatically

>
> --Rolland
> 
> De: Giuseppe D'Angelo <giuseppe.dang...@kdab.com>
> Envoyé: 27 oct. 2016 18:36
> À: Александр Волков; development@qt-project.org
> Objet: Re: [Development] Branch for adding Q_DECL_OVERRIDEs
>
>> Hi,
>>
>> please keep the discussion on the mailing list, so others can chime in.
>>
>> Il 27/10/2016 17:47, Александр Волков ha scritto:
>>> 27.10.2016 18:19, Giuseppe D'Angelo пишет:
>>>> Don't worry too much. To minimize merge conflicts I'm quite sure we'll
>>>> keep using Q_DECL_OVERRIDE as long as 5.6 will be open. So use that :)
>>>> We'll do a cleanup pass in the future.
>>>
>>> BTW, should 'override' be added in the following case:
>>> struct A {
>>>    virtual void f();
>>> };
>>> struct B : A {
>>>    void f() final; // override also?
>>> };
>>> ?
>>>
>>> Unfortunately gcc reports a warning for f()
>>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78010)
>>> There some such places in Qt, for example in
>>> corelib/kernel/qeventdispatcher_glib_p.h,
>>> where Q_DECL_OVERRIDE should be added to make it compilable with gcc
>>> [-Werror, -Wsuggest-override].
>>> The question is whether to follow a reasonable style
>>> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rh-override
>>> or a style forced by gcc?
>>
>> To be honest I don't know if we have a consensus on this. (How many
>> cases of "final" in our classes do we have anyhow?)
>>
>> Cheers,
>> --
>> Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer
>> KDAB (UK) Ltd., a KDAB Group company | Tel: UK +44-1625-809908
>> KDAB - Qt, C++ and OpenGL Experts
>
> ,
>
> ___
> Development mailing list
> Development@qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development


-- 
Regards,
Konstantin
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Branch for adding Q_DECL_OVERRIDEs

[Rolland Dudemaine]

Using the GHS compiler, i see thousands of examples of this.
To the point that (bad practice warning!) I disabled the warning.

--Rolland

De: Giuseppe D'Angelo <giuseppe.dang...@kdab.com>
Envoyé: 27 oct. 2016 18:36
À: Александр Волков; development@qt-project.org
Objet: Re: [Development] Branch for adding Q_DECL_OVERRIDEs


Hi,

please keep the discussion on the mailing list, so others can chime in.

Il 27/10/2016 17:47, Александр Волков ha scritto:
> 27.10.2016 18:19, Giuseppe D'Angelo пишет:
>> Don't worry too much. To minimize merge conflicts I'm quite sure we'll
>> keep using Q_DECL_OVERRIDE as long as 5.6 will be open. So use that :)
>> We'll do a cleanup pass in the future.
>
> BTW, should 'override' be added in the following case:
> struct A {
>virtual void f();
> };
> struct B : A {
>void f() final; // override also?
> };
> ?
>
> Unfortunately gcc reports a warning for f()
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78010)
> There some such places in Qt, for example in
> corelib/kernel/qeventdispatcher_glib_p.h,
> where Q_DECL_OVERRIDE should be added to make it compilable with gcc
> [-Werror, -Wsuggest-override].
> The question is whether to follow a reasonable style
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rh-override
> or a style forced by gcc?

To be honest I don't know if we have a consensus on this. (How many
cases of "final" in our classes do we have anyhow?)

Cheers,
--
Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer
KDAB (UK) Ltd., a KDAB Group company | Tel: UK +44-1625-809908
KDAB - Qt, C++ and OpenGL Experts

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Branch for adding Q_DECL_OVERRIDEs

[Giuseppe D'Angelo]

Hi,

please keep the discussion on the mailing list, so others can chime in.

Il 27/10/2016 17:47, Александр Волков ha scritto:
> 27.10.2016 18:19, Giuseppe D'Angelo пишет:
>> Don't worry too much. To minimize merge conflicts I'm quite sure we'll
>> keep using Q_DECL_OVERRIDE as long as 5.6 will be open. So use that :)
>> We'll do a cleanup pass in the future.
> 
> BTW, should 'override' be added in the following case:
> struct A {
>virtual void f();
> };
> struct B : A {
>void f() final; // override also?
> };
> ?
> 
> Unfortunately gcc reports a warning for f() 
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78010)
> There some such places in Qt, for example in 
> corelib/kernel/qeventdispatcher_glib_p.h,
> where Q_DECL_OVERRIDE should be added to make it compilable with gcc 
> [-Werror, -Wsuggest-override].
> The question is whether to follow a reasonable style
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rh-override
> or a style forced by gcc?

To be honest I don't know if we have a consensus on this. (How many
cases of "final" in our classes do we have anyhow?)

Cheers,
-- 
Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer
KDAB (UK) Ltd., a KDAB Group company | Tel: UK +44-1625-809908
KDAB - Qt, C++ and OpenGL Experts



smime.p7s
Description: Firma crittografica S/MIME
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Branch for adding Q_DECL_OVERRIDEs

[Giuseppe D'Angelo]

Il 27/10/2016 15:43, Александр Волков ha scritto:
> Are there plans about adding 'override' or 'Q_DECL_OVERRIDE'
> and which branch is preferred for such changes?

They should be added everywhere (because they cost nothing, and they
allow us to catch bugs), but they should especially added in places that
generate warnings (due to "inconsistent" usage).

The right branch for all of these fixes is the oldest branch which is 1)
still open, and 2) where the fix applies.

For qtsvg the open branches are 5.6, 5.7, 5.8, dev; so pick the oldest
one where your fix applies (likely 5.6).

Hope this helps,
-- 
Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer
KDAB (UK) Ltd., a KDAB Group company | Tel: UK +44-1625-809908
KDAB - Qt, C++ and OpenGL Experts



smime.p7s
Description: Firma crittografica S/MIME
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development