loplugin:unnecessarygetstr extend to more std::string checking

suggested by mike kaganski

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

2 files changed, 76 insertions, 60 deletions

diff --git a/compilerplugins/clang/test/unnecessarygetstr.cxx b/compilerplugins/clang/test/unnecessarygetstr.cxx
index 68ed153649ad..bb5fcc20d2ef 100644
--- a/compilerplugins/clang/test/unnecessarygetstr.cxx
+++ b/compilerplugins/clang/test/unnecessarygetstr.cxx
@@ -28,13 +28,13 @@ struct Foo
 void test1(Foo& foo)
 {
     OString s;
-    // expected-error@+1 {{unnecessary call to 'getStr' when passing to OString arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to OString constructor [loplugin:unnecessarygetstr]}}
     f1(true, s.getStr());
-    // expected-error@+1 {{unnecessary call to 'getStr' when passing to OString arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to OString constructor [loplugin:unnecessarygetstr]}}
     foo.f1(true, s.getStr());
-    // expected-error@+1 {{unnecessary call to 'getStr' when passing to OString arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to OString constructor [loplugin:unnecessarygetstr]}}
     foo.f1(true, OString::boolean(true).getStr());
-    // expected-error@+1 {{unnecessary call to 'getStr' when passing to OString arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to OString constructor [loplugin:unnecessarygetstr]}}
     foo.f1(true, OString::number(12).getStr());
 
     // avoid false +
@@ -49,22 +49,36 @@ namespace test2
 {
 // call to param that takes string_view
 void f2(bool, std::string_view);
+void f2(bool, std::u16string_view);
 struct Foo2
 {
     void f2(bool, std::string_view);
+    void f2(bool, std::u16string_view);
 };
-void test2(Foo2& foo)
+void testOString(Foo2& foo)
 {
     OString s;
-    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
     f2(true, s.getStr());
-    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
     foo.f2(true, s.getStr());
-    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
     foo.f2(true, OString::boolean(true).getStr());
-    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
     foo.f2(true, OString::number(12).getStr());
 }
+void testOUString(Foo2& foo)
+{
+    OUString s;
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
+    f2(true, s.getStr());
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
+    foo.f2(true, s.getStr());
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
+    foo.f2(true, OUString::boolean(true).getStr());
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
+    foo.f2(true, OUString::number(12).getStr());
+}
 }
 
 namespace test3
@@ -78,9 +92,9 @@ struct Foo2
 void test3(Foo2& foo)
 {
     std::string s;
-    // expected-error@+1 {{unnecessary call to 'c_str' when passing to string_view arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'c_str' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
     f2(true, s.c_str());
-    // expected-error@+1 {{unnecessary call to 'c_str' when passing to string_view arg [loplugin:unnecessarygetstr]}}
+    // expected-error@+1 {{unnecessary call to 'c_str' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
     foo.f2(true, s.c_str());
 }
 }
@@ -95,4 +109,15 @@ void test()
 }
 }
 
+namespace test5
+{
+void test(std::string v, OString o)
+{
+    // expected-error@+1 {{unnecessary call to 'c_str' when passing to string_view constructor [loplugin:unnecessarygetstr]}}
+    std::string_view s1(v.c_str());
+    // expected-error@+1 {{unnecessary call to 'getStr' when passing to string constructor [loplugin:unnecessarygetstr]}}
+    std::string s2(o.getStr());
+}
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/unnecessarygetstr.cxx b/compilerplugins/clang/unnecessarygetstr.cxx
index 3caf3776e5f7..7a9610f7618b 100644
--- a/compilerplugins/clang/unnecessarygetstr.cxx
+++ b/compilerplugins/clang/unnecessarygetstr.cxx
@@ -36,66 +36,52 @@ public:
     {
     }
 
-    bool VisitCallExpr(CallExpr* callExpr)
+    bool VisitCallExpr(const CallExpr* callExpr)
     {
         if (ignoreLocation(callExpr))
             return true;
         const FunctionDecl* func = callExpr->getDirectCallee();
         if (!func)
             return true;
-        unsigned const n = std::min(func->getNumParams(), callExpr->getNumArgs());
-        for (unsigned i = 0; i != n; ++i)
-        {
-            auto arg = callExpr->getArg(i);
-            if (auto matTemp = dyn_cast<MaterializeTemporaryExpr>(arg))
-            {
-                auto cxxConstruct = dyn_cast<CXXConstructExpr>(matTemp->IgnoreImplicit());
-                if (!cxxConstruct || cxxConstruct->getNumArgs() < 1
-                    || cxxConstruct->getNumArgs() > 2)
-                    continue;
-                auto const tc1 = loplugin::TypeCheck(cxxConstruct->getConstructor()->getParent());
-                if (!(tc1.Class("OString").Namespace("rtl").GlobalNamespace()
-                      || tc1.Class("OUString").Namespace("rtl").GlobalNamespace()))
-                    continue;
-                auto e = dyn_cast<CXXMemberCallExpr>(cxxConstruct->getArg(0)->IgnoreImplicit());
-                if (!e)
-                    continue;
-                auto const t = e->getObjectType();
-                auto const tc2 = loplugin::TypeCheck(t);
-                if (!(tc2.Class("OString").Namespace("rtl").GlobalNamespace()
-                      || tc2.Class("OUString").Namespace("rtl").GlobalNamespace()
-                      || tc2.Class("OStringBuffer").Namespace("rtl").GlobalNamespace()
-                      || tc2.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
-                      || tc2.ClassOrStruct("StringNumber").Namespace("rtl").GlobalNamespace()))
-                    continue;
-                if (!loplugin::DeclCheck(e->getMethodDecl()).Function("getStr"))
-                    continue;
-                report(DiagnosticsEngine::Warning,
-                       "unnecessary call to 'getStr' when passing to OString arg", e->getExprLoc())
-                    << e->getSourceRange();
-            }
-            else if (auto impCast = dyn_cast<ImplicitCastExpr>(arg))
-            {
-                auto cxxConstruct = dyn_cast<CXXConstructExpr>(impCast->getSubExpr());
-                if (!cxxConstruct || cxxConstruct->getNumArgs() < 1
-                    || cxxConstruct->getNumArgs() > 2)
-                    continue;
-                auto const tc1 = loplugin::TypeCheck(cxxConstruct->getConstructor()->getParent());
-                if (!(tc1.ClassOrStruct("basic_string_view").StdNamespace()))
-                    continue;
-                checkForGetStr(cxxConstruct->getArg(0), "string_view arg");
-            }
-        }
         if (loplugin::DeclCheck(func)
                 .Function("createFromAscii")
                 .Class("OUString")
-                .Namespace("rtl"))
+                .Namespace("rtl")
+                .GlobalNamespace())
         {
             checkForGetStr(callExpr->getArg(0), "OUString::createFromAscii");
         }
         return true;
     }
 
+    bool VisitCXXConstructExpr(const CXXConstructExpr* constructExpr)
+    {
+        if (ignoreLocation(constructExpr))
+            return true;
+        auto tc = loplugin::TypeCheck(constructExpr->getType());
+        if (tc.ClassOrStruct("basic_string").StdNamespace())
+        {
+            if (constructExpr->getNumArgs() == 2)
+                checkForGetStr(constructExpr->getArg(0), "string constructor");
+        }
+        else if (tc.ClassOrStruct("basic_string_view").StdNamespace())
+        {
+            if (constructExpr->getNumArgs() == 1)
+                checkForGetStr(constructExpr->getArg(0), "string_view constructor");
+        }
+        else if (tc.Class("OString").Namespace("rtl").GlobalNamespace())
+        {
+            if (constructExpr->getNumArgs() == 1 || constructExpr->getNumArgs() == 2)
+                checkForGetStr(constructExpr->getArg(0), "OString constructor");
+        }
+        else if (tc.Class("OUString").Namespace("rtl").GlobalNamespace())
+        {
+            if (constructExpr->getNumArgs() == 2)
+                checkForGetStr(constructExpr->getArg(0), "OUString constructor");
+        }
+        return true;
+    }
+
     bool preRun() override
     {
         if (!compiler.getLangOpts().CPlusPlus)
@@ -108,7 +94,7 @@ public:
     }
 
 private:
-    void checkForGetStr(Expr* arg, const char* msg)
+    void checkForGetStr(const Expr* arg, const char* msg)
     {
         auto e = dyn_cast<CXXMemberCallExpr>(arg->IgnoreImplicit());
         if (!e)
@@ -122,9 +108,14 @@ private:
             || tc2.ClassOrStruct("StringNumber").Namespace("rtl").GlobalNamespace())
         {
             if (loplugin::DeclCheck(e->getMethodDecl()).Function("getStr"))
-                report(DiagnosticsEngine::Warning,
-                       "unnecessary call to 'getStr' when passing to %0", e->getExprLoc())
-                    << msg << e->getSourceRange();
+            {
+                StringRef fileName = getFilenameOfLocation(
+                    compiler.getSourceManager().getSpellingLoc(e->getBeginLoc()));
+                if (!loplugin::hasPathnamePrefix(fileName, SRCDIR "/include/rtl/"))
+                    report(DiagnosticsEngine::Warning,
+                           "unnecessary call to 'getStr' when passing to %0", e->getExprLoc())
+                        << msg << e->getSourceRange();
+            }
         }
         else if (tc2.Class("basic_string").StdNamespace())
         {