Re: [Libreoffice-commits] core.git: clang-tidy performance-move-const-arg

On 06/09/18 08:33, Libreoffice Gerrit user wrote:
> commit 3d604d1cd6cc70ef96782ef769f0479b509987a8
> Author:     Noel Grandin <noel.grandin@collabora.co.uk>
> AuthorDate: Wed Sep 5 13:51:39 2018 +0200
> Commit:     Noel Grandin <noel.grandin@collabora.co.uk>
> CommitDate: Thu Sep 6 08:33:28 2018 +0200
>
>      clang-tidy performance-move-const-arg
>      
>      Change-Id: I607891e120688b746c8a4c577018d97147a79217
>      Reviewed-on: https://gerrit.libreoffice.org/60029
>      Tested-by: Jenkins
>      Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
[...]
> diff --git a/editeng/source/editeng/eertfpar.cxx b/editeng/source/editeng/eertfpar.cxx
> index ccecfa5723de..16f202d24a28 100644
> --- a/editeng/source/editeng/eertfpar.cxx
> +++ b/editeng/source/editeng/eertfpar.cxx
> @@ -62,14 +62,14 @@ RtfImportInfo::~RtfImportInfo()
>   {
>   }
>   
> -EditRTFParser::EditRTFParser(
> -    SvStream& rIn, EditSelection aSel, SfxItemPool& rAttrPool, EditEngine* pEditEngine) :
> -    SvxRTFParser(rAttrPool, rIn),
> -    aCurSel(std::move(aSel)),
> -    mpEditEngine(pEditEngine),
> -    aRTFMapMode(MapUnit::MapTwip),
> -    nDefFont(0),
> -    bLastActionInsertParaBreak(false)
> +EditRTFParser::EditRTFParser(SvStream& rIn, EditSelection aSel, SfxItemPool& rAttrPool,
> +                             EditEngine* pEditEngine)
> +    : SvxRTFParser(rAttrPool, rIn)
> +    , aCurSel(aSel)
> +    , mpEditEngine(pEditEngine)
> +    , aRTFMapMode(MapUnit::MapTwip)
> +    , nDefFont(0)
> +    , bLastActionInsertParaBreak(false)
>   {
>       SetInsPos(EditPosition(mpEditEngine, &aCurSel));
>   

I assume dropping the std::move from aCurSel(std::move(aSel)) is caused 
by performance-move-const-arg's warning "if std::move() is called with 
an argument of a trivially-copyable type" 
(<https://clang.llvm.org/extra/clang-tidy/checks/performance-move-const-arg.html>).

For my taste, that approach is too tightly tied to a class's current 
implementation details, something that may change over time.  Imagine a 
trivially copyable class C with a raw pointer member (where the 
lifecycle of the pointed-to object is tracked by some higher-level, 
likely broken protocol).  Now, that protocol is fixed and the raw 
pointer member is replaced with a std::shared_ptr.  C is no longer 
trivially copyable, and the dropped std::move would turn out to be 
beneficial again.

(Also, if Clang and clang-tidy did implement the fixed rules for 
trivially copyable classes from CWG1734/C++17, where a subset of a 
trivially copyable class's copy/move members may be deleted, the above 
rule to warn "if std::move() is called with an argument of a 
trivially-copyable type" would no longer make sense as written; consider 
a trivially copyable class whose copy ctor has been deleted.)

(And, concerning this specific commit:  Reformatting unrelated and 
otherwise unchanged lines is a pain.  It makes it so much more difficult 
to spot the actual changes here.)