improve loplugin:stringconstant

to find more places we can elide the OUString() constructor at call
sites

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

2 files changed, 76 insertions, 45 deletions

diff --git a/compilerplugins/clang/stringconstant.cxx b/compilerplugins/clang/stringconstant.cxx
index 05cfa03ff711..6a009e510297 100644
--- a/compilerplugins/clang/stringconstant.cxx
+++ b/compilerplugins/clang/stringconstant.cxx
@@ -58,7 +58,19 @@ bool isLhsOfAssignment(FunctionDecl const * decl, unsigned parameter) {
         || (oo >= OO_PlusEqual && oo <= OO_GreaterGreaterEqual);
 }
 
-bool hasOverloads(FunctionDecl const * decl, unsigned arguments) {
+bool typecheckIsOUStringParam(const clang::QualType t) {
+    return bool(loplugin::TypeCheck(t).NotSubstTemplateTypeParmType()
+            .LvalueReference().Const().NotSubstTemplateTypeParmType()
+            .Class("OUString").Namespace("rtl").GlobalNamespace());
+}
+
+bool typecheckIsOStringParam(const clang::QualType t) {
+    return bool(loplugin::TypeCheck(t).NotSubstTemplateTypeParmType()
+            .LvalueReference().Const().NotSubstTemplateTypeParmType()
+            .Class("OString").Namespace("rtl").GlobalNamespace());
+}
+
+bool hasOverloads(FunctionDecl const * decl, unsigned arguments, unsigned paramIndex) {
     int n = 0;
     auto ctx = decl->getDeclContext();
     if (ctx->getDeclKind() == Decl::LinkageSpec) {
@@ -67,17 +79,32 @@ bool hasOverloads(FunctionDecl const * decl, unsigned arguments) {
     auto res = ctx->lookup(decl->getDeclName());
     for (auto d = res.begin(); d != res.end(); ++d) {
         FunctionDecl const * f = dyn_cast<FunctionDecl>(*d);
-        if (f != nullptr && f->getMinRequiredArguments() <= arguments
-            && f->getNumParams() >= arguments)
-        {
-            auto consDecl = dyn_cast<CXXConstructorDecl>(f);
-            if (consDecl && consDecl->isCopyOrMoveConstructor()) {
-                continue;
-            }
-            ++n;
-            if (n == 2) {
-                return true;
-            }
+        if (f == nullptr || f->getMinRequiredArguments() > arguments
+            || f->getNumParams() < arguments) {
+            continue;
+        }
+        auto consDecl = dyn_cast<CXXConstructorDecl>(f);
+        if (consDecl && consDecl->isCopyOrMoveConstructor()) {
+            continue;
+        }
+        // Deleted stuff like in ORowSetValueDecorator in connectivity can cause
+        // trouble.
+        if (consDecl && consDecl->isDeleted()) {
+            return true;
+        }
+        if (paramIndex >= f->getNumParams()) {
+            continue;
+        }
+        auto t = f->getParamDecl(paramIndex)->getType();
+        // bool because 'const char *' converts to bool
+        if (!typecheckIsOUStringParam(t) && !typecheckIsOStringParam(t)
+            && !loplugin::TypeCheck(t).Pointer().Const().Char()
+            && !loplugin::TypeCheck(t).AnyBoolean()) {
+            continue;
+        }
+        ++n;
+        if (n == 2) {
+            return true;
         }
     }
     return false;
@@ -269,29 +296,6 @@ bool StringConstant::VisitCallExpr(CallExpr const * expr) {
     if (fdecl == nullptr) {
         return true;
     }
-    for (unsigned i = 0; i != fdecl->getNumParams(); ++i) {
-        auto t = fdecl->getParamDecl(i)->getType();
-        if (loplugin::TypeCheck(t).NotSubstTemplateTypeParmType()
-            .LvalueReference().Const().NotSubstTemplateTypeParmType()
-            .Class("OUString").Namespace("rtl").GlobalNamespace())
-        {
-            if (!(isLhsOfAssignment(fdecl, i)
-                  || hasOverloads(fdecl, expr->getNumArgs())))
-            {
-                handleOUStringCtor(expr, i, fdecl, true);
-            }
-        }
-        if (loplugin::TypeCheck(t).NotSubstTemplateTypeParmType()
-            .LvalueReference().Const().NotSubstTemplateTypeParmType()
-            .Class("OString").Namespace("rtl").GlobalNamespace())
-        {
-            if (!(isLhsOfAssignment(fdecl, i)
-                  || hasOverloads(fdecl, expr->getNumArgs())))
-            {
-                handleOStringCtor(expr, i, fdecl, true);
-            }
-        }
-    }
     loplugin::DeclCheck dc(fdecl);
     //TODO: u.compareToAscii("foo") -> u.???("foo")
     //TODO: u.compareToIgnoreAsciiCaseAscii("foo") -> u.???("foo")
@@ -773,6 +777,25 @@ bool StringConstant::VisitCallExpr(CallExpr const * expr) {
         }
         return true;
     }
+    for (unsigned i = 0; i != fdecl->getNumParams(); ++i) {
+        auto t = fdecl->getParamDecl(i)->getType();
+        if (typecheckIsOUStringParam(t))
+        {
+            if (!(isLhsOfAssignment(fdecl, i)
+                  || hasOverloads(fdecl, expr->getNumArgs(), i)))
+            {
+                handleOUStringCtor(expr, i, fdecl, true);
+            }
+        }
+        if (typecheckIsOStringParam(t))
+        {
+            if (!(isLhsOfAssignment(fdecl, i)
+                  || hasOverloads(fdecl, expr->getNumArgs(), i)))
+            {
+                handleOStringCtor(expr, i, fdecl, true);
+            }
+        }
+    }
     return true;
 }
 
@@ -1176,27 +1199,23 @@ bool StringConstant::VisitCXXConstructExpr(CXXConstructExpr const * expr) {
     auto consDecl = expr->getConstructor();
     for (unsigned i = 0; i != consDecl->getNumParams(); ++i) {
         auto t = consDecl->getParamDecl(i)->getType();
-        if (loplugin::TypeCheck(t).NotSubstTemplateTypeParmType()
-            .LvalueReference().Const().NotSubstTemplateTypeParmType()
-            .Class("OUString").Namespace("rtl").GlobalNamespace())
+        if (typecheckIsOUStringParam(t))
         {
             auto argExpr = expr->getArg(i);
             if (argExpr && i <= consDecl->getNumParams())
             {
-                if (!hasOverloads(consDecl, expr->getNumArgs()))
+                if (!hasOverloads(consDecl, expr->getNumArgs(), i))
                 {
                     handleOUStringCtor(expr, argExpr, consDecl, true);
                 }
             }
         }
-        if (loplugin::TypeCheck(t).NotSubstTemplateTypeParmType()
-            .LvalueReference().Const().NotSubstTemplateTypeParmType()
-            .Class("OString").Namespace("rtl").GlobalNamespace())
+        if (typecheckIsOStringParam(t))
         {
             auto argExpr = expr->getArg(i);
             if (argExpr && i <= consDecl->getNumParams())
             {
-                if (!hasOverloads(consDecl, expr->getNumArgs()))
+                if (!hasOverloads(consDecl, expr->getNumArgs(), i))
                 {
                     handleOStringCtor(expr, argExpr, consDecl, true);
                 }
diff --git a/compilerplugins/clang/test/stringconstant.cxx b/compilerplugins/clang/test/stringconstant.cxx
index 49ae3b68d035..1eda24580ab2 100644
--- a/compilerplugins/clang/test/stringconstant.cxx
+++ b/compilerplugins/clang/test/stringconstant.cxx
@@ -17,6 +17,7 @@
 extern void foo(OUString const &);
 
 struct Foo {
+    Foo(int, const OUString &) {}
     Foo(OUString const &, int) {}
     Foo(OUString const &) {}
     void foo(OUString const &) const {}
@@ -28,6 +29,11 @@ struct Foo2 {
     void foo(OString const &) const {}
 };
 
+struct NegativeFoo {
+    NegativeFoo(const OString&, const OString& ) {}
+    NegativeFoo(const OString&, const OUString& ) {}
+};
+
 int main() {
     char const s1[] = "foo";
     char const * const s2 = "foo";
@@ -67,9 +73,15 @@ int main() {
     (void)aFoo;
     Foo aFoo2(OUString("xxx")); // expected-error {{in call of 'Foo::Foo', replace 'OUString' constructed from a string literal directly with the string literal}}
     aFoo2.foo(OUString("xxx")); // expected-error {{in call of 'Foo::foo', replace 'OUString' constructed from a string literal directly with the string literal}}
+    Foo aFoo3(1, OUString("xxx")); // expected-error {{in call of 'Foo::Foo', replace 'OUString' constructed from a string literal directly with the string literal}}
+    (void)aFoo3;
+
+    Foo2 aFoo4(OString("xxx")); // expected-error {{in call of 'Foo2::Foo2', replace 'OUString' constructed from a string literal directly with the string literal}}
+    aFoo4.foo(OString("xxx")); // expected-error {{in call of 'Foo2::foo', replace 'OUString' constructed from a string literal directly with the string literal}}
 
-    Foo2 aFoo3(OString("xxx")); // expected-error {{in call of 'Foo2::Foo2', replace 'OUString' constructed from a string literal directly with the string literal}}
-    aFoo3.foo(OString("xxx")); // expected-error {{in call of 'Foo2::foo', replace 'OUString' constructed from a string literal directly with the string literal}}
+    // no warning expected
+    NegativeFoo aNegativeFoo(OString("xxx"), OUString("1"));
+    (void)aNegativeFoo;
 
     (void) OUString("xxx", 3, RTL_TEXTENCODING_ASCII_US); // expected-error {{simplify construction of 'OUString' with string constant argument [loplugin:stringconstant]}}
     (void) OUString("xxx", 3, RTL_TEXTENCODING_ISO_8859_1); // expected-error {{suspicious 'rtl::OUString' constructor with text encoding 12 but plain ASCII content; use 'RTL_TEXTENCODING_ASCII_US' instead [loplugin:stringconstant]}}