loplugin:stringconstant look for unnecessary OString constructor use

and tweak the methods in check.hxx to make them more flexible when
called with
dc.Class(xxx ? "foo" : "bar")

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

3 files changed, 103 insertions, 57 deletions

diff --git a/compilerplugins/clang/check.hxx b/compilerplugins/clang/check.hxx
index 407258393eb8..4ae65f139955 100644
--- a/compilerplugins/clang/check.hxx
+++ b/compilerplugins/clang/check.hxx
@@ -24,8 +24,8 @@ class TerminalCheck;
 
 namespace detail {
 
-template<std::size_t N> ContextCheck checkRecordDecl(
-    clang::Decl const * decl, clang::TagTypeKind tag, char const (& id)[N]);
+inline ContextCheck checkRecordDecl(
+    clang::Decl const * decl, clang::TagTypeKind tag, llvm::StringRef id);
 
 }
 
@@ -59,16 +59,13 @@ public:
 
     TypeCheck LvalueReference() const;
 
-    template<std::size_t N> inline ContextCheck Class(char const (& id)[N])
-        const;
+    inline ContextCheck Class(llvm::StringRef id) const;
 
-    template<std::size_t N> inline ContextCheck Struct(char const (& id)[N])
-        const;
+    inline ContextCheck Struct(llvm::StringRef id) const;
 
     TypeCheck Typedef() const;
 
-    template<std::size_t N> inline ContextCheck Typedef(char const (& id)[N])
-        const;
+    inline ContextCheck Typedef(llvm::StringRef id) const;
 
     TypeCheck NotSubstTemplateTypeParmType() const;
 
@@ -84,22 +81,17 @@ public:
 
     explicit operator bool() const { return decl_ != nullptr; }
 
-    template<std::size_t N> inline ContextCheck Class(char const (& id)[N])
-        const;
+    inline ContextCheck Class(llvm::StringRef id) const;
 
-    template<std::size_t N> inline ContextCheck Struct(char const (& id)[N])
-        const;
+    inline ContextCheck Struct(llvm::StringRef id) const;
 
-    template<std::size_t N> inline ContextCheck Union(char const (& id)[N])
-        const;
+    inline ContextCheck Union(llvm::StringRef id) const;
 
-    template<std::size_t N> inline ContextCheck Function(char const (& id)[N])
-        const;
+    inline ContextCheck Function(llvm::StringRef id) const;
 
     ContextCheck Operator(clang::OverloadedOperatorKind op) const;
 
-    template<std::size_t N> inline ContextCheck Var(char const (& id)[N])
-        const;
+    inline ContextCheck Var(llvm::StringRef id) const;
 
     ContextCheck MemberFunction() const;
 
@@ -113,24 +105,21 @@ public:
 
     TerminalCheck GlobalNamespace() const;
 
-    template<std::size_t N> inline ContextCheck Namespace(
-        char const (& id)[N]) const;
+    inline ContextCheck Namespace(llvm::StringRef id) const;
 
     TerminalCheck StdNamespace() const;
 
     ContextCheck AnonymousNamespace() const;
 
-    template<std::size_t N> inline ContextCheck Class(char const (& id)[N])
-        const;
+    inline ContextCheck Class(llvm::StringRef id) const;
 
-    template<std::size_t N> inline ContextCheck Struct(char const (& id)[N])
-        const;
+    inline ContextCheck Struct(llvm::StringRef id) const;
 
 private:
     friend DeclCheck;
     friend TypeCheck;
-    template<std::size_t N> friend ContextCheck detail::checkRecordDecl(
-        clang::Decl const * decl, clang::TagTypeKind tag, char const (& id)[N]);
+    friend ContextCheck detail::checkRecordDecl(
+        clang::Decl const * decl, clang::TagTypeKind tag, llvm::StringRef id);
 
     explicit ContextCheck(clang::DeclContext const * context = nullptr):
         context_(context) {}
@@ -153,13 +142,13 @@ private:
 
 namespace detail {
 
-template<std::size_t N> ContextCheck checkRecordDecl(
-    clang::Decl const * decl, clang::TagTypeKind tag, char const (& id)[N])
+ContextCheck checkRecordDecl(
+    clang::Decl const * decl, clang::TagTypeKind tag, llvm::StringRef id)
 {
     auto r = llvm::dyn_cast_or_null<clang::RecordDecl>(decl);
     if (r != nullptr && r->getTagKind() == tag) {
         auto const i = r->getIdentifier();
-        if (i != nullptr && i->isStr(id)) {
+        if (i != nullptr && i->getName() == id) {
             return ContextCheck(r->getDeclContext());
         }
     }
@@ -168,7 +157,7 @@ template<std::size_t N> ContextCheck checkRecordDecl(
 
 }
 
-template<std::size_t N> ContextCheck TypeCheck::Class(char const (& id)[N])
+ContextCheck TypeCheck::Class(llvm::StringRef id)
     const
 {
     if (!type_.isNull()) {
@@ -180,8 +169,7 @@ template<std::size_t N> ContextCheck TypeCheck::Class(char const (& id)[N])
     return ContextCheck();
 }
 
-template<std::size_t N> ContextCheck TypeCheck::Struct(char const (& id)[N])
-    const
+ContextCheck TypeCheck::Struct(llvm::StringRef id) const
 {
     if (!type_.isNull()) {
         auto const t = type_->getAs<clang::RecordType>();
@@ -192,15 +180,14 @@ template<std::size_t N> ContextCheck TypeCheck::Struct(char const (& id)[N])
     return ContextCheck();
 }
 
-template<std::size_t N> ContextCheck TypeCheck::Typedef(char const (& id)[N])
-    const
+ContextCheck TypeCheck::Typedef(llvm::StringRef id) const
 {
     if (!type_.isNull()) {
         if (auto const t = type_->getAs<clang::TypedefType>()) {
             auto const d = t->getDecl();
             auto const i = d->getIdentifier();
             assert(i != nullptr);
-            if (i->isStr(id)) {
+            if (i->getName() == id) {
                 return ContextCheck(d->getDeclContext());
             }
         }
@@ -208,58 +195,52 @@ template<std::size_t N> ContextCheck TypeCheck::Typedef(char const (& id)[N])
     return ContextCheck();
 }
 
-template<std::size_t N> ContextCheck DeclCheck::Class(char const (& id)[N])
-    const
+ContextCheck DeclCheck::Class(llvm::StringRef id) const
 {
     return detail::checkRecordDecl(decl_, clang::TTK_Class, id);
 }
 
-template<std::size_t N> ContextCheck DeclCheck::Struct(char const (& id)[N])
-    const
+ContextCheck DeclCheck::Struct(llvm::StringRef id) const
 {
     return detail::checkRecordDecl(decl_, clang::TTK_Struct, id);
 }
 
-template<std::size_t N> ContextCheck DeclCheck::Union(char const (& id)[N])
-    const
+ContextCheck DeclCheck::Union(llvm::StringRef id) const
 {
     return detail::checkRecordDecl(decl_, clang::TTK_Union, id);
 }
 
-template<std::size_t N> ContextCheck DeclCheck::Function(char const (& id)[N])
-    const
+ContextCheck DeclCheck::Function(llvm::StringRef id) const
 {
     auto f = llvm::dyn_cast_or_null<clang::FunctionDecl>(decl_);
     if (f != nullptr) {
         auto const i = f->getIdentifier();
-        if (i != nullptr && i->isStr(id)) {
+        if (i != nullptr && i->getName() == id) {
             return ContextCheck(f->getDeclContext());
         }
     }
     return ContextCheck();
 }
 
-template<std::size_t N> ContextCheck DeclCheck::Var(char const (& id)[N])
-    const
+ContextCheck DeclCheck::Var(llvm::StringRef id) const
 {
     auto f = llvm::dyn_cast_or_null<clang::VarDecl>(decl_);
     if (f != nullptr) {
         auto const i = f->getIdentifier();
-        if (i != nullptr && i->isStr(id)) {
+        if (i != nullptr && i->getName() == id) {
             return ContextCheck(f->getDeclContext());
         }
     }
     return ContextCheck();
 }
 
-template<std::size_t N> ContextCheck ContextCheck::Namespace(
-    char const (& id)[N]) const
+ContextCheck ContextCheck::Namespace(llvm::StringRef id) const
 {
     if (context_) {
         auto n = llvm::dyn_cast<clang::NamespaceDecl>(context_);
         if (n != nullptr) {
             auto const i = n->getIdentifier();
-            if (i != nullptr && i->isStr(id)) {
+            if (i != nullptr && i->getName() == id) {
                 return ContextCheck(n->getParent());
             }
         }
@@ -267,15 +248,13 @@ template<std::size_t N> ContextCheck ContextCheck::Namespace(
     return ContextCheck();
 }
 
-template<std::size_t N> ContextCheck ContextCheck::Class(char const (& id)[N])
-    const
+ContextCheck ContextCheck::Class(llvm::StringRef id) const
 {
     return detail::checkRecordDecl(
         llvm::dyn_cast_or_null<clang::Decl>(context_), clang::TTK_Class, id);
 }
 
-template<std::size_t N> ContextCheck ContextCheck::Struct(char const (& id)[N])
-    const
+ContextCheck ContextCheck::Struct(llvm::StringRef id) const
 {
     return detail::checkRecordDecl(
         llvm::dyn_cast_or_null<clang::Decl>(context_), clang::TTK_Struct, id);
diff --git a/compilerplugins/clang/stringconstant.cxx b/compilerplugins/clang/stringconstant.cxx
index dd4eeff3763a..83a62a617d06 100644
--- a/compilerplugins/clang/stringconstant.cxx
+++ b/compilerplugins/clang/stringconstant.cxx
@@ -163,10 +163,23 @@ private:
         CallExpr const * expr, unsigned arg, FunctionDecl const * callee,
         bool explicitFunctionalCastNotation);
 
+    void handleOStringCtor(
+        CallExpr const * expr, unsigned arg, FunctionDecl const * callee,
+        bool explicitFunctionalCastNotation);
+
     void handleOUStringCtor(
         Expr const * expr, Expr const * argExpr, FunctionDecl const * callee,
         bool explicitFunctionalCastNotation);
 
+    void handleOStringCtor(
+        Expr const * expr, Expr const * argExpr, FunctionDecl const * callee,
+        bool explicitFunctionalCastNotation);
+
+    enum class StringKind { Unicode, Char };
+    void handleStringCtor(
+        Expr const * expr, Expr const * argExpr, FunctionDecl const * callee,
+        bool explicitFunctionalCastNotation, StringKind stringKind);
+
     void handleFunArgOstring(
         CallExpr const * expr, unsigned arg, FunctionDecl const * callee);
 
@@ -268,6 +281,16 @@ bool StringConstant::VisitCallExpr(CallExpr const * expr) {
                 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")
@@ -1166,6 +1189,19 @@ bool StringConstant::VisitCXXConstructExpr(CXXConstructExpr const * expr) {
                 }
             }
         }
+        if (loplugin::TypeCheck(t).NotSubstTemplateTypeParmType()
+            .LvalueReference().Const().NotSubstTemplateTypeParmType()
+            .Class("OString").Namespace("rtl").GlobalNamespace())
+        {
+            auto argExpr = expr->getArg(i);
+            if (argExpr && i <= consDecl->getNumParams())
+            {
+                if (!hasOverloads(consDecl, expr->getNumArgs()))
+                {
+                    handleOStringCtor(expr, argExpr, consDecl, true);
+                }
+            }
+        }
     }
 
     return true;
@@ -1785,10 +1821,31 @@ void StringConstant::handleOUStringCtor(
     handleOUStringCtor(expr, expr->getArg(arg), callee, explicitFunctionalCastNotation);
 }
 
+void StringConstant::handleOStringCtor(
+    CallExpr const * expr, unsigned arg, FunctionDecl const * callee,
+    bool explicitFunctionalCastNotation)
+{
+    handleOStringCtor(expr, expr->getArg(arg), callee, explicitFunctionalCastNotation);
+}
+
 void StringConstant::handleOUStringCtor(
     Expr const * expr, Expr const * argExpr, FunctionDecl const * callee,
     bool explicitFunctionalCastNotation)
 {
+    handleStringCtor(expr, argExpr, callee, explicitFunctionalCastNotation, StringKind::Unicode);
+}
+
+void StringConstant::handleOStringCtor(
+    Expr const * expr, Expr const * argExpr, FunctionDecl const * callee,
+    bool explicitFunctionalCastNotation)
+{
+    handleStringCtor(expr, argExpr, callee, explicitFunctionalCastNotation, StringKind::Char);
+}
+
+void StringConstant::handleStringCtor(
+    Expr const * expr, Expr const * argExpr, FunctionDecl const * callee,
+    bool explicitFunctionalCastNotation, StringKind stringKind)
+{
     auto e0 = argExpr->IgnoreParenImpCasts();
     auto e1 = dyn_cast<CXXFunctionalCastExpr>(e0);
     if (e1 == nullptr) {
@@ -1808,7 +1865,7 @@ void StringConstant::handleOUStringCtor(
         return;
     }
     if (!loplugin::DeclCheck(e3->getConstructor()).MemberFunction()
-        .Class("OUString").Namespace("rtl").GlobalNamespace())
+        .Class(stringKind == StringKind::Unicode ? "OUString" : "OString").Namespace("rtl").GlobalNamespace())
     {
         return;
     }
@@ -1825,7 +1882,7 @@ void StringConstant::handleOUStringCtor(
         && e3->getConstructor()->getNumParams() == 1
         && (loplugin::TypeCheck(
                 e3->getConstructor()->getParamDecl(0)->getType())
-            .Typedef("sal_Unicode").GlobalNamespace()))
+            .Typedef(stringKind == StringKind::Unicode ? "sal_Unicode" : "char").GlobalNamespace()))
     {
         // It may not be easy to rewrite OUString(c), esp. given there is no
         // OUString ctor taking an OUStringLiteral1 arg, so don't warn there:
diff --git a/compilerplugins/clang/test/stringconstant.cxx b/compilerplugins/clang/test/stringconstant.cxx
index 068d34f5078d..49ae3b68d035 100644
--- a/compilerplugins/clang/test/stringconstant.cxx
+++ b/compilerplugins/clang/test/stringconstant.cxx
@@ -19,6 +19,13 @@ extern void foo(OUString const &);
 struct Foo {
     Foo(OUString const &, int) {}
     Foo(OUString const &) {}
+    void foo(OUString const &) const {}
+};
+
+struct Foo2 {
+    Foo2(OString const &, int) {}
+    Foo2(OString const &) {}
+    void foo(OString const &) const {}
 };
 
 int main() {
@@ -59,7 +66,10 @@ int main() {
     Foo aFoo(OUString("xxx"), 1); // expected-error {{in call of 'Foo::Foo', replace 'OUString' constructed from a string literal directly with the string literal}}
     (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}}
-    (void)aFoo2;
+    aFoo2.foo(OUString("xxx")); // expected-error {{in call of 'Foo::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}}
 
     (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]}}