loplugin:passstuffbyref improved return in canvas and svtools

and for now, ignore methods with params so we don't fall into the trap
of thinking that calls to methods like:
   Bar& foo(Bar &p) { return p; }
can be converted from
   Bar f() { return foo(Bar()); }
to
Bar const & f() { return foo(Bar()); }

Change-Id: Ia3795eb2baf353cb6bec4ebf40451f2789d66ad7
Reviewed-on: https://gerrit.libreoffice.org/47034Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

canvas/source/cairo/cairo_canvasfont.cxx

@@ -146,7 +146,7 @@ namespace cairocanvas
         return { "com.sun.star.rendering.CanvasFont" };
     }
 
-    vcl::Font CanvasFont::getVCLFont() const
+    vcl::Font const & CanvasFont::getVCLFont() const
     {
         return *maFont;
     }

canvas/source/cairo/cairo_canvasfont.hxx

@@ -72,7 +72,7 @@ namespace cairocanvas
         virtual sal_Bool SAL_CALL supportsService( const OUString& ServiceName ) override;
         virtual css::uno::Sequence< OUString > SAL_CALL getSupportedServiceNames() override;
 
-        vcl::Font getVCLFont() const;
+        vcl::Font const & getVCLFont() const;
 
     private:
         ::canvas::vcltools::VCLObject<vcl::Font> maFont;

canvas/source/cairo/cairo_devicehelper.cxx

@@ -223,7 +223,7 @@ namespace cairocanvas
         };
     }
 
-    uno::Reference<rendering::XColorSpace> DeviceHelper::getColorSpace() const
+    uno::Reference<rendering::XColorSpace> const & DeviceHelper::getColorSpace() const
     {
         // always the same
         return DeviceColorSpace::get();

canvas/source/cairo/cairo_devicehelper.hxx

@@ -83,7 +83,7 @@ namespace cairocanvas
         css::uno::Any getDeviceHandle() const;
         css::uno::Any getSurfaceHandle() const;
         css::uno::Reference<
-            css::rendering::XColorSpace > getColorSpace() const;
+            css::rendering::XColorSpace > const & getColorSpace() const;
 
         /** called when DumpScreenContent property is enabled on
             XGraphicDevice, and writes out bitmaps of current screen.

canvas/source/cairo/cairo_spritecanvas.cxx

@@ -173,12 +173,12 @@ namespace cairocanvas
         return maDeviceHelper.getOutputDevice();
     }
 
-    SurfaceSharedPtr SpriteCanvas::getBufferSurface()
+    SurfaceSharedPtr const & SpriteCanvas::getBufferSurface()
     {
         return maDeviceHelper.getBufferSurface();
     }
 
-    SurfaceSharedPtr SpriteCanvas::getWindowSurface()
+    SurfaceSharedPtr const & SpriteCanvas::getWindowSurface()
     {
         return maDeviceHelper.getWindowSurface();
     }

canvas/source/cairo/cairo_spritecanvas.hxx

@@ -142,8 +142,8 @@ namespace cairocanvas
                               const css::rendering::ViewState&   viewState,
                               const css::rendering::RenderState& renderState ) override;
 
-        ::cairo::SurfaceSharedPtr getWindowSurface();
-        ::cairo::SurfaceSharedPtr getBufferSurface();
+        ::cairo::SurfaceSharedPtr const & getWindowSurface();
+        ::cairo::SurfaceSharedPtr const & getBufferSurface();
 
         const ::basegfx::B2ISize& getSizePixel();
         void setSizePixel( const ::basegfx::B2ISize& rSize );

canvas/source/cairo/cairo_spritedevicehelper.cxx

@@ -124,7 +124,7 @@ namespace cairocanvas
         setSize( ::basegfx::B2ISize(rBounds.Width, rBounds.Height) );
     }
 
-    SurfaceSharedPtr SpriteDeviceHelper::getWindowSurface()
+    SurfaceSharedPtr const & SpriteDeviceHelper::getWindowSurface()
     {
         return DeviceHelper::getSurface();
     }

canvas/source/cairo/cairo_spritedevicehelper.hxx

@@ -61,7 +61,7 @@ namespace cairocanvas
         void setSize( const ::basegfx::B2ISize& rSize );
 
         const ::cairo::SurfaceSharedPtr& getBufferSurface() { return mpBufferSurface; }
-        ::cairo::SurfaceSharedPtr getWindowSurface();
+        ::cairo::SurfaceSharedPtr const & getWindowSurface();
         ::cairo::SurfaceSharedPtr createSurface( const ::basegfx::B2ISize& rSize, int aContent );
         ::cairo::SurfaceSharedPtr createSurface( BitmapSystemData const & rData, const Size& rSize );
         const ::basegfx::B2ISize& getSizePixel() { return maSize; }

canvas/source/tools/canvastools.cxx

@@ -863,12 +863,12 @@ namespace canvas
             };
         }
 
-        uno::Reference<rendering::XIntegerBitmapColorSpace> getStdColorSpace()
+        uno::Reference<rendering::XIntegerBitmapColorSpace> const & getStdColorSpace()
         {
             return StandardColorSpaceHolder::get();
         }
 
-        uno::Reference<rendering::XIntegerBitmapColorSpace> getStdColorSpaceWithoutAlpha()
+        uno::Reference<rendering::XIntegerBitmapColorSpace> const & getStdColorSpaceWithoutAlpha()
         {
             return StandardNoAlphaColorSpaceHolder::get();
         }

canvas/source/vcl/canvasfont.cxx

@@ -159,7 +159,7 @@ namespace vclcanvas
         return { "com.sun.star.rendering.CanvasFont" };
     }
 
-    vcl::Font CanvasFont::getVCLFont() const
+    vcl::Font const & CanvasFont::getVCLFont() const
     {
         return *maFont;
     }

canvas/source/vcl/canvasfont.hxx

@@ -75,7 +75,7 @@ namespace vclcanvas
         virtual sal_Bool SAL_CALL supportsService( const OUString& ServiceName ) override;
         virtual css::uno::Sequence< OUString > SAL_CALL getSupportedServiceNames() override;
 
-        vcl::Font getVCLFont() const;
+        vcl::Font const & getVCLFont() const;
 
     private:
         ::canvas::vcltools::VCLObject<vcl::Font>                          maFont;

canvas/source/vcl/devicehelper.cxx

@@ -189,7 +189,7 @@ namespace vclcanvas
         };
     }
 
-    uno::Reference<rendering::XColorSpace> DeviceHelper::getColorSpace() const
+    uno::Reference<rendering::XColorSpace> const & DeviceHelper::getColorSpace() const
     {
         // always the same
         return DeviceColorSpace::get();

canvas/source/vcl/devicehelper.hxx

@@ -75,7 +75,7 @@ namespace vclcanvas
         css::uno::Any getDeviceHandle() const;
         css::uno::Any getSurfaceHandle() const;
         css::uno::Reference<
-            css::rendering::XColorSpace > getColorSpace() const;
+            css::rendering::XColorSpace > const & getColorSpace() const;
 
         const OutDevProviderSharedPtr& getOutDev() const { return mpOutDev; }
 

canvas/source/vcl/spritecanvas.hxx

@@ -143,9 +143,9 @@ namespace vclcanvas
                               const GraphicAttr&                              rAttr ) const override;
 
         /// Get backbuffer for this canvas
-        OutDevProviderSharedPtr getFrontBuffer() const { return maDeviceHelper.getOutDev(); }
+        OutDevProviderSharedPtr const & getFrontBuffer() const { return maDeviceHelper.getOutDev(); }
         /// Get window for this canvas
-        BackBufferSharedPtr getBackBuffer() const { return maDeviceHelper.getBackBuffer(); }
+        BackBufferSharedPtr const & getBackBuffer() const { return maDeviceHelper.getBackBuffer(); }
 
     private:
         css::uno::Sequence< css::uno::Any >                maArguments;

compilerplugins/clang/passstuffbyref.cxx

@@ -402,6 +402,12 @@ bool PassStuffByRef::isReturnExprDisqualified(const Expr* expr)
             FunctionDecl const * calleeFunctionDecl = callExpr->getDirectCallee();
             if (!calleeFunctionDecl)
                 return true;
+            // TODO anything takes a param is suspect because it might return the param by ref.
+            // we could tighten this to only reject functions that have a param of the same type
+            // as the return type. Or we could check for such functions and disallow them.
+            // Or we could force such functions to be annotated somehow.
+            if (calleeFunctionDecl->getNumParams() > 0)
+                return true;
             auto tc = loplugin::TypeCheck(calleeFunctionDecl->getReturnType());
             if (!tc.LvalueReference() && !tc.Pointer())
                 return true;

compilerplugins/clang/test/passstuffbyref.cxx

@@ -15,6 +15,7 @@
 struct S1 {
     OUString mv1;
     OUString const & get() const { return mv1; }
+    OUString const & get2(bool) const { return mv1; }
 };
 struct S2 {
     OUString mv1;
@@ -40,6 +41,8 @@ struct S2 {
     // TODO
     OUString get10() { return OUString(*&get6()); } // todoexpected-error {{rather return class rtl::OUString by const& than by value, to avoid unnecessary copying [loplugin:passstuffbyref]}}
     OUString get11() const { return mxCow->get(); } // expected-error {{rather return class rtl::OUString by const& than by value, to avoid unnecessary copying [loplugin:passstuffbyref]}}
+    // TODO anything takes a param is suspect because it might return the param by ref
+    OUString get12() { return child.get2(false); } // todoexpected-error {{rather return class rtl::OUString by const& than by value, to avoid unnecessary copying [loplugin:passstuffbyref]}}
 
     // no warning expected
     OUString set1() { return OUString("xxx"); }

include/canvas/canvastools.hxx

@@ -329,7 +329,7 @@ namespace canvas
             Use this method for dead-simple bitmap implementations,
             that map all their formats to 8888 RGBA color.
          */
-        CANVASTOOLS_DLLPUBLIC css::uno::Reference< css::rendering::XIntegerBitmapColorSpace> getStdColorSpace();
+        CANVASTOOLS_DLLPUBLIC css::uno::Reference< css::rendering::XIntegerBitmapColorSpace> const & getStdColorSpace();
 
         /** Return a color space for a default RGB integer format
 
@@ -337,7 +337,7 @@ namespace canvas
             that map all their formats to 8888 RGB color (the last byte
             is unused).
          */
-        CANVASTOOLS_DLLPUBLIC css::uno::Reference< css::rendering::XIntegerBitmapColorSpace> getStdColorSpaceWithoutAlpha();
+        CANVASTOOLS_DLLPUBLIC css::uno::Reference< css::rendering::XIntegerBitmapColorSpace> const & getStdColorSpaceWithoutAlpha();
 
         /** Return a memory layout for a default RGBA integer format
 

include/svtools/helpopt.hxx

@@ -44,7 +44,7 @@ public:
     const OUString& GetHelpStyleSheet()const;
     void            SetHelpStyleSheet(const OUString& rStyleSheet);
 
-    OUString        GetSystem() const;
+    OUString const & GetSystem() const;
 };
 
 #endif

include/svtools/hyperlabel.hxx

@@ -69,7 +69,7 @@ namespace svt
 
         void                SetClickHdl( const Link<HyperLabel*,void>& rLink ) { maClickHdl = rLink; }
 
-        Size                CalcMinimumSize( long nMaxWidth ) const;
+        Size const &        CalcMinimumSize( long nMaxWidth ) const;
     };
 }
 

include/svtools/treelistbox.hxx

@@ -697,7 +697,7 @@ public:
 
     void            SetCollapsedNodeBmp( const Image& );
     void            SetExpandedNodeBmp( const Image& );
-    Image           GetExpandedNodeBmp( ) const;
+    Image const &   GetExpandedNodeBmp( ) const;
 
     void            SetFont( const vcl::Font& rFont );
 
@@ -796,7 +796,7 @@ public:
     void        LoseFocus();
     bool        EditingCanceled() const { return bCanceled; }
     OUString    GetText() const;
-    OUString    GetSavedValue() const;
+    OUString const & GetSavedValue() const;
     void        StopEditing( bool bCancel );
     void        Hide();
 };

svtools/inc/roadmap.hxx

@@ -72,7 +72,7 @@ public:
     bool            SelectRoadmapItemByID( ItemId _nItemID );
 
     void            SetItemSelectHdl( const Link<LinkParamNone*,void>& _rHdl );
-    Link<LinkParamNone*,void> GetItemSelectHdl( ) const;
+    Link<LinkParamNone*,void> const & GetItemSelectHdl( ) const;
     virtual void    DataChanged( const DataChangedEvent& rDCEvt ) override;
     virtual void    GetFocus() override;
     virtual void    ApplySettings( vcl::RenderContext& rRenderContext ) override;

svtools/source/config/helpopt.cxx

@@ -279,7 +279,7 @@ bool SvtHelpOptions::IsHelpTips() const
     return pImpl->IsHelpTips();
 }
 
-OUString SvtHelpOptions::GetSystem() const
+OUString const & SvtHelpOptions::GetSystem() const
 {
     return pImpl->GetSystem();
 }

svtools/source/contnr/treelistbox.cxx

@@ -140,7 +140,7 @@ SvInplaceEdit2::~SvInplaceEdit2()
     pEdit.disposeAndClear();
 }
 
-OUString SvInplaceEdit2::GetSavedValue() const
+OUString const & SvInplaceEdit2::GetSavedValue() const
 {
     return pEdit->GetSavedValue();
 }
@@ -2420,7 +2420,7 @@ void SvTreeListBox::SetCurEntry( SvTreeListEntry* pEntry )
     pImpl->SetCurEntry( pEntry );
 }
 
-Image SvTreeListBox::GetExpandedNodeBmp( ) const
+Image const & SvTreeListBox::GetExpandedNodeBmp( ) const
 {
     return pImpl->GetExpandedNodeBmp( );
 }

svtools/source/control/hyperlabel.cxx

@@ -54,7 +54,7 @@ namespace svt
         implInit();
     }
 
-    Size HyperLabel::CalcMinimumSize( long nMaxWidth ) const
+    Size const & HyperLabel::CalcMinimumSize( long nMaxWidth ) const
     {
         m_pImpl->m_aMinSize = FixedText::CalcMinimumSize( nMaxWidth );
         // the MinimumSize is used to size the FocusRectangle

svtools/source/control/roadmap.cxx

@@ -507,7 +507,7 @@ void ORoadmap::SetItemSelectHdl(const Link<LinkParamNone*,void>& _rHdl)
     m_pImpl->setSelectHdl(_rHdl);
 }
 
-Link<LinkParamNone*,void> ORoadmap::GetItemSelectHdl() const
+Link<LinkParamNone*,void> const & ORoadmap::GetItemSelectHdl() const
 {
     return m_pImpl->getSelectHdl();
 }