Re: [Libreoffice-commits] core.git: cppcheck style fix for noExplicitConstructor in writerfilter

classic Classic list List threaded Threaded
5 messages Options
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: cppcheck style fix for noExplicitConstructor in writerfilter

On 12/03/2016 07:10 PM, Jochen Nitschke wrote:
> commit 76936e787bd13fb1a747b7c716df3fba2d0d3fa9
> Author: Jochen Nitschke <[hidden email]>
> Date:   Sat Dec 3 13:39:44 2016 +0100
>
>     cppcheck style fix for noExplicitConstructor in writerfilter
>
>     make ctors with one parameter explicit

Seeing this and similar commits mentioning "cppcheck" and
"noExplicitConstructor" made me curious:

* Is cppcheck reporting each and every ctor (with one parameter?
callable with one argument?) that isn't declared as explicit?  Or does
it use some heuristic that might be actually useful?

* Why that concentration on single-parameter ctors?  Is cppcheck a
pre-C++11 tool?  That leads to an arbitrary-looking mix of explicit and
non-explicit ctors in cases like

[...]

> diff --git a/writerfilter/source/rtftok/rtfvalue.hxx b/writerfilter/source/rtftok/rtfvalue.hxx
> index eeb9730..d113fbf 100644
> --- a/writerfilter/source/rtftok/rtfvalue.hxx
> +++ b/writerfilter/source/rtftok/rtfvalue.hxx
> @@ -32,14 +32,14 @@ public:
>               css::uno::Reference<css::embed::XEmbeddedObject> const& xObject,
>               bool bForceString, const RTFShape& aShape);
>      RTFValue();
> -    RTFValue(int nValue);
> +    explicit RTFValue(int nValue);
>      RTFValue(const OUString& sValue, bool bForce = false);
> -    RTFValue(RTFSprms rAttributes);
> +    explicit RTFValue(RTFSprms rAttributes);
>      RTFValue(RTFSprms rAttributes, RTFSprms rSprms);
> -    RTFValue(css::uno::Reference<css::drawing::XShape> const& xShape);
> -    RTFValue(css::uno::Reference<css::io::XInputStream> const& xStream);
> -    RTFValue(css::uno::Reference<css::embed::XEmbeddedObject> const& xObject);
> -    RTFValue(const RTFShape& aShape);
> +    explicit RTFValue(css::uno::Reference<css::drawing::XShape> const& xShape);
> +    explicit RTFValue(css::uno::Reference<css::io::XInputStream> const& xStream);
> +    explicit RTFValue(css::uno::Reference<css::embed::XEmbeddedObject> const& xObject);
> +    explicit RTFValue(const RTFShape& aShape);
>      virtual ~RTFValue() override;
>      void setString(const OUString& sValue);
>      virtual int getInt() const override;
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
j.nitschke@ok.de j.nitschke@ok.de
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: cppcheck style fix for noExplicitConstructor in writerfilter

On 12/05/2016 10:58 AM, Stephan Bergmann wrote:

> On 12/03/2016 07:10 PM, Jochen Nitschke wrote:
>> commit 76936e787bd13fb1a747b7c716df3fba2d0d3fa9
>> Author: Jochen Nitschke <[hidden email]>
>> Date:   Sat Dec 3 13:39:44 2016 +0100
>>
>>     cppcheck style fix for noExplicitConstructor in writerfilter
>>
>>     make ctors with one parameter explicit
>
> Seeing this and similar commits mentioning "cppcheck" and
> "noExplicitConstructor" made me curious:
>
> * Is cppcheck reporting each and every ctor (with one parameter?
> callable with one argument?) that isn't declared as explicit?  Or does
> it use some heuristic that might be actually useful?
So far I saw only ctors with one parameter (none with one mandatory plus
x optional parameters). There seems to be no heuristic other than
excluding copy and move ctors.

from cppcheck source:

>             // We are looking for constructors, which are meeting
> following criteria:
>             //  1) Constructor is declared with a single parameter
>             //  2) Constructor is not declared as explicit
>             //  3) It is not a copy/move constructor of non-abstract class
>             //  4) Constructor is not marked as delete (programmer can
> mark the default constructor as deleted, which is ok)
>             if (!func->isConstructor() || func->isDelete() ||
> (!func->hasBody() && func->access == Private))
>                 continue;
>
>             if (!func->isExplicit() &&
>                 func->argCount() == 1 &&
>                 func->type != Function::eCopyConstructor &&
>                 func->type != Function::eMoveConstructor) {
> noExplicitConstructorError(func->tokenDef, scope->className,
> scope->type == Scope::eStruct);


> * Why that concentration on single-parameter ctors?  Is cppcheck a
> pre-C++11 tool?  That leads to an arbitrary-looking mix of explicit
> and non-explicit ctors in cases like
This is simply following guides like the C++ Core Guidelines. [1]
I think the reason for the rule didn't went away with C++11. But with {
initialisation } there is a new good use case now.

Sadly cppcheck results have a lot of noise, ~20% are such
noExplicitConstructor informations.
Maybe disable these messages for future reports. [2]

[1]
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-explicit
[2] http://dev-builds.libreoffice.org/cppcheck_reports/master/

> [...]
>> diff --git a/writerfilter/source/rtftok/rtfvalue.hxx
>> b/writerfilter/source/rtftok/rtfvalue.hxx
>> index eeb9730..d113fbf 100644
>> --- a/writerfilter/source/rtftok/rtfvalue.hxx
>> +++ b/writerfilter/source/rtftok/rtfvalue.hxx
>> @@ -32,14 +32,14 @@ public:
>>               css::uno::Reference<css::embed::XEmbeddedObject> const&
>> xObject,
>>               bool bForceString, const RTFShape& aShape);
>>      RTFValue();
>> -    RTFValue(int nValue);
>> +    explicit RTFValue(int nValue);
>>      RTFValue(const OUString& sValue, bool bForce = false);
>> -    RTFValue(RTFSprms rAttributes);
>> +    explicit RTFValue(RTFSprms rAttributes);
>>      RTFValue(RTFSprms rAttributes, RTFSprms rSprms);
>> -    RTFValue(css::uno::Reference<css::drawing::XShape> const& xShape);
>> -    RTFValue(css::uno::Reference<css::io::XInputStream> const&
>> xStream);
>> -    RTFValue(css::uno::Reference<css::embed::XEmbeddedObject> const&
>> xObject);
>> -    RTFValue(const RTFShape& aShape);
>> +    explicit RTFValue(css::uno::Reference<css::drawing::XShape>
>> const& xShape);
>> +    explicit RTFValue(css::uno::Reference<css::io::XInputStream>
>> const& xStream);
>> +    explicit
>> RTFValue(css::uno::Reference<css::embed::XEmbeddedObject> const&
>> xObject);
>> +    explicit RTFValue(const RTFShape& aShape);
>>      virtual ~RTFValue() override;
>>      void setString(const OUString& sValue);
>>      virtual int getInt() const override;

PS: could someone look into the cppcheck warnings:
accessForwarded - Access of forwarded variable expression
accessMoved - Access of moved variable pReleasedFormat.
--
www.Ok.de - die kostenlose E-Mail Adresse
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: cppcheck style fix for noExplicitConstructor in writerfilter

On 12/05/2016 02:00 PM, [hidden email] wrote:
> On 12/05/2016 10:58 AM, Stephan Bergmann wrote:
>> * Why that concentration on single-parameter ctors?  Is cppcheck a
>> pre-C++11 tool?  That leads to an arbitrary-looking mix of explicit
>> and non-explicit ctors in cases like
> This is simply following guides like the C++ Core Guidelines. [1]
> I think the reason for the rule didn't went away with C++11. But with {
> initialisation } there is a new good use case now.

Hm, [1] states "Single-argument constructors should be declared
explicit.  Good single argument non-explicit constructors are rare in
most code based [sic]."  No idea whether or not that's true; just found
it puzzling that cppcheck would warn about each (non-copy/move) ctor
with one parameter, without any further heuristics.  (And why doesn't it
also warn about ctors callable with one argument?  Probably an oversight.)

> [1]
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-explicit
> [2] http://dev-builds.libreoffice.org/cppcheck_reports/master/

> PS: could someone look into the cppcheck warnings:

I find one each of those in [2]:

> accessForwarded - Access of forwarded variable expression

<http://dev-builds.libreoffice.org/cppcheck_reports/master/466.html#line-63>
is harmless.  What's relevant in the OUStringBuffer ctor call is not the
content of the passed argument (which is ignored anyway by the special
RTL_STRING_UNITTEST ctors) but only its type.

> accessMoved - Access of moved variable pReleasedFormat.

<http://dev-builds.libreoffice.org/cppcheck_reports/master/664.html#line-4657>
looks OK, too:  SwDoc::DelTableStyle will either store pReleasedFormat
in a new SwUndo and return an empty std::unique_ptr, or return
pReleasedFormat.  That code was added by
<https://cgit.freedesktop.org/libreoffice/core/commit/?id=0943ee2decb8d5a1a2a5bf3b1c233934a89e9c97>
"GSoC Writer Table Styles; Create by example; fix undo"---any further
thoughts from its author or committer (on cc)?
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: cppcheck style fix for noExplicitConstructor in writerfilter

On 12/06/2016 11:19 AM, Stephan Bergmann wrote:
>> accessForwarded - Access of forwarded variable expression
>
> <http://dev-builds.libreoffice.org/cppcheck_reports/master/466.html#line-63>
> is harmless.  What's relevant in the OUStringBuffer ctor call is not the
> content of the passed argument (which is ignored anyway by the special
> RTL_STRING_UNITTEST ctors) but only its type.

<https://cgit.freedesktop.org/libreoffice/core/commit/?id=ba8e95d0e86682349f377480e65fbf4641ef27f2>
"Comment cppcheck accessForwarded as harmless".  There's no way to mark
this as a false positive for future cppcheck runs, or is there?
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Miklos Vajna-4 Miklos Vajna-4
Reply | Threaded
Open this post in threaded view
|

Re: [Libreoffice-commits] core.git: cppcheck style fix for noExplicitConstructor in writerfilter

In reply to this post by sberg
Hi,

On Tue, Dec 06, 2016 at 11:19:56AM +0100, Stephan Bergmann <[hidden email]> wrote:
> >accessMoved - Access of moved variable pReleasedFormat.
>
> <http://dev-builds.libreoffice.org/cppcheck_reports/master/664.html#line-4657>
> looks OK, too:  SwDoc::DelTableStyle will either store pReleasedFormat in a
> new SwUndo and return an empty std::unique_ptr, or return pReleasedFormat.
> That code was added by <https://cgit.freedesktop.org/libreoffice/core/commit/?id=0943ee2decb8d5a1a2a5bf3b1c233934a89e9c97>
> "GSoC Writer Table Styles; Create by example; fix undo"---any further
> thoughts from its author or committer (on cc)?

Hm yes, warning on that unconditionally sounds like a way to generate
lots of false positives. ;-)

As said above, the member function returns nullptr exactly when the
format is already used for undo purposes, I don't see a problem with
that approach.

Regards,

Miklos

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice

signature.asc (188 bytes) Download Attachment