loplugin:refcounting return objects properly

check that when we return ref-counted objects, we do so using
rtl::Reference, so that the object actually has a non-zero
ref count.

Change-Id: Ib3ffae0d2502f6d117550c82fde5449729c27324
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/111487
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

3 files changed, 42 insertions, 1 deletions

diff --git a/compilerplugins/clang/refcounting.cxx b/compilerplugins/clang/refcounting.cxx
index 7f1d7f38fba8..319a23b83a9b 100644
--- a/compilerplugins/clang/refcounting.cxx
+++ b/compilerplugins/clang/refcounting.cxx
@@ -58,6 +58,7 @@ public:
     bool VisitTypeLoc(clang::TypeLoc typeLoc);
     bool VisitCXXDeleteExpr(const CXXDeleteExpr *);
     bool VisitBinaryOperator(const BinaryOperator *);
+    bool VisitReturnStmt(const ReturnStmt *);
 
     // Creation of temporaries with one argument are represented by
     // CXXFunctionalCastExpr, while any other number of arguments are
@@ -520,6 +521,7 @@ bool RefCounting::VisitCXXDeleteExpr(const CXXDeleteExpr * cxxDeleteExpr)
     }
     return true;
 }
+
 bool RefCounting::VisitFieldDecl(const FieldDecl * fieldDecl) {
     if (ignoreLocation(fieldDecl)) {
         return true;
@@ -599,6 +601,33 @@ bool RefCounting::VisitFieldDecl(const FieldDecl * fieldDecl) {
     return true;
 }
 
+bool RefCounting::VisitReturnStmt(const ReturnStmt * returnStmt) {
+    if (ignoreLocation(returnStmt)) {
+        return true;
+    }
+
+    if (!returnStmt->getRetValue())
+        return true;
+    auto cxxNewExpr = dyn_cast<CXXNewExpr>(compat::IgnoreImplicit(returnStmt->getRetValue()));
+    if (!cxxNewExpr)
+        return true;
+
+    auto qt = returnStmt->getRetValue()->getType();
+    if (!qt->isPointerType())
+        return false;
+    qt = qt->getPointeeType();
+
+    if (containsOWeakObjectSubclass(qt)) {
+        report(
+            DiagnosticsEngine::Warning,
+            "new object of cppu::OWeakObject subclass %0 being returned via raw pointer, should be returned by via rtl::Reference",
+            compat::getBeginLoc(returnStmt))
+            << qt
+            << returnStmt->getSourceRange();
+    }
+
+    return true;
+}
 
 bool RefCounting::VisitVarDecl(const VarDecl * varDecl) {
     if (ignoreLocation(varDecl))
diff --git a/compilerplugins/clang/test/refcounting.cxx b/compilerplugins/clang/test/refcounting.cxx
index ca27ac0614a7..4c133023b0b8 100644
--- a/compilerplugins/clang/test/refcounting.cxx
+++ b/compilerplugins/clang/test/refcounting.cxx
@@ -18,6 +18,7 @@ namespace cppu
 {
 class OWeakObject
 {
+public:
     void acquire();
     void release();
 };
@@ -83,4 +84,15 @@ void foo4()
     p = new UnoObject;
 }
 
+UnoObject* foo5()
+{
+    // expected-error@+1 {{new object of cppu::OWeakObject subclass 'UnoObject' being returned via raw pointer, should be returned by via rtl::Reference [loplugin:refcounting]}}
+    return new UnoObject;
+}
+rtl::Reference<UnoObject> foo6()
+{
+    // no warning expected
+    return new UnoObject;
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/dbaccess/source/core/dataaccess/databasedocument.hxx b/dbaccess/source/core/dataaccess/databasedocument.hxx
index 6ce35092ea5b..d4deb94b2186 100644
--- a/dbaccess/source/core/dataaccess/databasedocument.hxx
+++ b/dbaccess/source/core/dataaccess/databasedocument.hxx
@@ -286,7 +286,7 @@ protected:
 
 public:
     struct FactoryAccess { friend class ODatabaseModelImpl; private: FactoryAccess() { } };
-    static ODatabaseDocument* createDatabaseDocument( const ::rtl::Reference<ODatabaseModelImpl>& _pImpl, FactoryAccess /*accessControl*/ )
+    static rtl::Reference<ODatabaseDocument> createDatabaseDocument( const ::rtl::Reference<ODatabaseModelImpl>& _pImpl, FactoryAccess /*accessControl*/ )
     {
         return new ODatabaseDocument( _pImpl );
     }