loplugin:stringadd also check O[U]StringBuffers

For similar code sequences that can be improved. Also move containsComment from collapseif plugin code to plugin.cxx so we can use it from stringadd. Change-Id: Ie07d9aedf2c31cb0b2080e1b8584294d7046a8e1 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/149217 Tested-by: Jenkins Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
2023-04-06 13:51:06 +02:00
parent d5673ec3f8
commit bd17c9c949
7 changed files with 246 additions and 147 deletions
@@ -38,7 +38,6 @@ public:

 private:
    int getNoCharsInSourceCodeOfExpr(IfStmt const*);
-    bool containsComment(Stmt const* stmt);
 };

 bool CollapseIf::VisitIfStmt(IfStmt const* ifStmt)
@@ -73,7 +72,7 @@ bool CollapseIf::VisitIfStmt(IfStmt const* ifStmt)

    // Sometimes there is a comment between the first and second if, so
    // merging them would make the comment more awkward to write.
-    if (containsComment(ifStmt))
+    if (containsComment(ifStmt->getSourceRange()))
        return true;

    report(DiagnosticsEngine::Warning, "nested if should be collapsed into one statement %0 %1",
@@ -101,27 +100,6 @@ int CollapseIf::getNoCharsInSourceCodeOfExpr(IfStmt const* ifStmt)
    return count;
 }

-bool CollapseIf::containsComment(Stmt const* stmt)
-{
-    SourceManager& SM = compiler.getSourceManager();
-    auto range = stmt->getSourceRange();
-    SourceLocation startLoc = range.getBegin();
-    SourceLocation endLoc = range.getEnd();
-    char const* p1 = SM.getCharacterData(startLoc);
-    char const* p2 = SM.getCharacterData(endLoc);
-    p2 += Lexer::MeasureTokenLength(endLoc, SM, compiler.getLangOpts());
-
-    // check for comments
-    constexpr char const comment1[] = "/*";
-    constexpr char const comment2[] = "//";
-    if (std::search(p1, p2, comment1, comment1 + strlen(comment1)) != p2)
-        return true;
-    if (std::search(p1, p2, comment2, comment2 + strlen(comment2)) != p2)
-        return true;
-
-    return false;
-}
-
 /** Off by default because some places are a judgement call if it should be collapsed or not. */
 loplugin::Plugin::Registration<CollapseIf> X("collapseif", false);
 }
@@ -543,6 +543,32 @@ bool Plugin::containsPreprocessingConditionalInclusion(SourceRange range)
    return false;
 }

+bool Plugin::containsComment(SourceRange range)
+{
+    SourceManager& SM = compiler.getSourceManager();
+    SourceLocation startLoc = range.getBegin();
+    SourceLocation endLoc = range.getEnd();
+    char const* p1 = SM.getCharacterData(startLoc);
+    char const* p2 = SM.getCharacterData(endLoc);
+    p2 += Lexer::MeasureTokenLength(endLoc, SM, compiler.getLangOpts());
+
+    // when doing 'make solenv.check' we don't want the special comments in the
+    // unit test files to trigger this check
+    constexpr char const comment0[] = "// expected-error";
+    if (std::search(p1, p2, comment0, comment0 + strlen(comment0)) != p2)
+        return false;
+
+    // check for comments
+    constexpr char const comment1[] = "/*";
+    constexpr char const comment2[] = "//";
+    if (std::search(p1, p2, comment1, comment1 + strlen(comment1)) != p2)
+        return true;
+    if (std::search(p1, p2, comment2, comment2 + strlen(comment2)) != p2)
+        return true;
+
+    return false;
+}
+
 Plugin::IdenticalDefaultArgumentsResult Plugin::checkIdenticalDefaultArguments(
    Expr const * argument1, Expr const * argument2)
 {
@@ -102,6 +102,8 @@ protected:

    bool containsPreprocessingConditionalInclusion(SourceRange range);

+    bool containsComment(SourceRange range);
+
    enum class IdenticalDefaultArgumentsResult { No, Yes, Maybe };
    IdenticalDefaultArgumentsResult checkIdenticalDefaultArguments(
        Expr const * argument1, Expr const * argument2);
@@ -21,7 +21,7 @@
 #include "clang/AST/StmtVisitor.h"

 /**
-    Look for repeated addition to OUString/OString.
+    Look for repeated addition to OUString/OString/OUStringBuffer/OStringBuffer.

    Eg.
      OUString x = "xxx";
@@ -61,6 +61,9 @@ public:
        // TODO the += depends on the result of the preceding assign, so can't merge
        if (fn == SRCDIR "/editeng/source/misc/svxacorr.cxx")
            return false;
+        // TODO this file has a boatload of buffer appends' and I don't feel like fixing them all now
+        if (fn == SRCDIR "/vcl/source/gdi/pdfwriter_impl.cxx")
+            return false;
        return true;
    }

@@ -143,12 +146,26 @@ StringAdd::VarDeclAndSummands StringAdd::findAssignOrAdd(Stmt const* stmt)
            {
                auto tc = loplugin::TypeCheck(varDeclLHS->getType());
                if (!tc.Class("OUString").Namespace("rtl").GlobalNamespace()
-                    && !tc.Class("OString").Namespace("rtl").GlobalNamespace())
+                    && !tc.Class("OString").Namespace("rtl").GlobalNamespace()
+                    && !tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
+                    && !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
                    return {};
                if (varDeclLHS->getStorageDuration() == SD_Static)
                    return {};
                if (!varDeclLHS->hasInit())
                    return {};
+                if (tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
+                    || tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
+                {
+                    // ignore the constructor that gives the buffer a default size
+                    if (auto cxxConstructor = dyn_cast<CXXConstructExpr>(varDeclLHS->getInit()))
+                        if (auto constructorDecl = cxxConstructor->getConstructor())
+                            if (constructorDecl->getNumParams() == 1
+                                && loplugin::TypeCheck(constructorDecl->getParamDecl(0)->getType())
+                                       .Typedef("sal_Int32")
+                                       .GlobalNamespace())
+                                return {};
+                }
                return { varDeclLHS, (isCompileTimeConstant(varDeclLHS->getInit())
                                          ? Summands::OnlyCompileTimeConstants
                                          : (isSideEffectFree(varDeclLHS->getInit())
@@ -171,6 +188,24 @@ StringAdd::VarDeclAndSummands StringAdd::findAssignOrAdd(Stmt const* stmt)
                                  : (isSideEffectFree(rhs) ? Summands::OnlySideEffectFree
                                                           : Summands::SideEffect)) };
                }
+    if (auto memberCall = dyn_cast<CXXMemberCallExpr>(stmt))
+        if (auto cxxMethodDecl = dyn_cast_or_null<CXXMethodDecl>(memberCall->getDirectCallee()))
+            if (cxxMethodDecl->getIdentifier() && cxxMethodDecl->getName() == "append")
+                if (auto declRefExprLHS
+                    = dyn_cast<DeclRefExpr>(ignore(memberCall->getImplicitObjectArgument())))
+                    if (auto varDeclLHS = dyn_cast<VarDecl>(declRefExprLHS->getDecl()))
+                    {
+                        auto tc = loplugin::TypeCheck(varDeclLHS->getType());
+                        if (!tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
+                            && !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
+                            return {};
+                        auto rhs = memberCall->getArg(0);
+                        return { varDeclLHS,
+                                 (isCompileTimeConstant(rhs)
+                                      ? Summands::OnlyCompileTimeConstants
+                                      : (isSideEffectFree(rhs) ? Summands::OnlySideEffectFree
+                                                               : Summands::SideEffect)) };
+                    }
    return {};
 }

@@ -182,20 +217,41 @@ bool StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2,
        stmt2 = exprCleanup->getSubExpr();
    if (auto switchCase = dyn_cast<SwitchCase>(stmt2))
        stmt2 = switchCase->getSubStmt();
-    auto operatorCall = dyn_cast<CXXOperatorCallExpr>(stmt2);
-    if (!operatorCall)
-        return false;
-    if (operatorCall->getOperator() != OO_PlusEqual)
-        return false;
-    auto declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(operatorCall->getArg(0)));
+
+    const DeclRefExpr* declRefExprLHS;
+    const Expr* rhs;
+    auto tc = loplugin::TypeCheck(varDecl.varDecl->getType());
+    if (tc.Class("OString") || tc.Class("OUString"))
+    {
+        auto operatorCall = dyn_cast<CXXOperatorCallExpr>(stmt2);
+        if (!operatorCall)
+            return false;
+        if (operatorCall->getOperator() != OO_PlusEqual)
+            return false;
+        declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(operatorCall->getArg(0)));
+        rhs = operatorCall->getArg(1);
+    }
+    else
+    {
+        // OUStringBuffer, OStringBuffer
+        auto memberCall = dyn_cast<CXXMemberCallExpr>(stmt2);
+        if (!memberCall)
+            return false;
+        auto cxxMethodDecl = dyn_cast_or_null<CXXMethodDecl>(memberCall->getDirectCallee());
+        if (!cxxMethodDecl)
+            return false;
+        if (!cxxMethodDecl->getIdentifier() || cxxMethodDecl->getName() != "append")
+            return false;
+        declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(memberCall->getImplicitObjectArgument()));
+        rhs = memberCall->getArg(0);
+    }
    if (!declRefExprLHS)
        return false;
    if (declRefExprLHS->getDecl() != varDecl.varDecl)
        return false;
    // if either side is a compile-time-constant, then we don't care about
    // side-effects
-    auto rhs = operatorCall->getArg(1);
-    auto const ctcRhs = isCompileTimeConstant(rhs);
+    bool const ctcRhs = isCompileTimeConstant(rhs);
    if (!ctcRhs)
    {
        auto const sefRhs = isSideEffectFree(rhs);
@@ -207,16 +263,23 @@ bool StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2,
            return true;
        }
    }
+    SourceRange mergeRange(stmt1->getSourceRange().getBegin(), stmt2->getSourceRange().getEnd());
    // if we cross a #ifdef boundary
-    if (containsPreprocessingConditionalInclusion(
-            SourceRange(stmt1->getSourceRange().getBegin(), stmt2->getSourceRange().getEnd())))
+    if (containsPreprocessingConditionalInclusion(mergeRange))
    {
        varDecl.summands
            = ctcRhs ? Summands::OnlyCompileTimeConstants
                     : isSideEffectFree(rhs) ? Summands::OnlySideEffectFree : Summands::SideEffect;
        return true;
    }
-    report(DiagnosticsEngine::Warning, "simplify by merging with the preceding assignment",
+    // If there is a comment between two calls, rather don't suggest merge
+    // IMO, code clarity trumps efficiency (as far as plugin warnings go, anyway).
+    if (containsComment(mergeRange))
+        return true;
+    // I don't think the OUStringAppend functionality can handle this efficiently
+    if (isa<ConditionalOperator>(ignore(rhs)))
+        return false;
+    report(DiagnosticsEngine::Warning, "simplify by merging with the preceding assign/append",
           stmt2->getBeginLoc())
        << stmt2->getSourceRange();
    return true;
@@ -419,7 +482,9 @@ bool StringAdd::isSideEffectFree(Expr const* expr)
    if (auto constructExpr = dyn_cast<CXXConstructExpr>(expr))
    {
        auto dc = loplugin::DeclCheck(constructExpr->getConstructor());
-        if (dc.MemberFunction().Class("OUString") || dc.MemberFunction().Class("OString"))
+        if (dc.MemberFunction().Class("OUString") || dc.MemberFunction().Class("OString")
+            || dc.MemberFunction().Class("OUStringBuffer")
+            || dc.MemberFunction().Class("OStringBuffer"))
            if (constructExpr->getNumArgs() == 0 || isSideEffectFree(constructExpr->getArg(0)))
                return true;
        // Expr::HasSideEffects does not like stuff that passes through OUStringLiteral
@@ -27,48 +27,48 @@ static const char XXX2[] = "xxx";
 void f1(OUString s1, int i, OString o)
 {
    OUString s2 = s1;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += s1;
    s2 = s1 + "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += s1;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += OUString::number(i);
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += XXX1;
    // expected-error-re@+2 {{rather use O[U]String::Concat than constructing '{{(rtl::)?}}OUStringLiteral<4>'{{( \(aka 'rtl::OUStringLiteral<4>'\))?}} from 'const char16_t{{ ?}}[4]' on LHS of + (where RHS is of type 'const char{{ ?}}[4]') [loplugin:stringadd]}}
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += OUStringLiteral(XXX1u) + XXX2;

-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += OStringToOUString(o, RTL_TEXTENCODING_UTF8);
 }
 void f2(OString s1, int i, OUString u)
 {
    OString s2 = s1;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += s1;
    s2 = s1 + "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += s1;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += OString::number(i);

-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += OUStringToOString(u, RTL_TEXTENCODING_ASCII_US);
 }
 void f3(OUString aStr, int nFirstContent)
 {
    OUString aFirstStr = aStr.copy(0, nFirstContent);
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    aFirstStr += "...";
 }
 OUString side_effect();
@@ -76,15 +76,15 @@ void f4(int i)
 {
    OUString s1;
    OUString s2("xxx");
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += "xxx";
    ++i;
    // any other kind of statement breaks the chain (at least for now)
    s2 += "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s2 += side_effect();
    s1 += "yyy";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s1 += "yyy";
 }
 }
@@ -94,13 +94,13 @@ namespace test2
 void f(OUString s3)
 {
    s3 += "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s3 += "xxx";
 }
 void g(OString s3)
 {
    s3 += "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s3 += "xxx";
 }
 }
@@ -114,28 +114,28 @@ struct Bar
 void f(Bar b1, Bar& b2, Bar* b3)
 {
    OUString s3 = "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s3 += b1.m_field;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s3 += b2.m_field;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s3 += b3->m_field;
 }
 OUString side_effect();
 void f2(OUString s)
 {
    OUString sRet = "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    sRet += side_effect();
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    sRet += "xxx";
    sRet += side_effect();
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    sRet += "xxx";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    sRet += "xxx";
    sRet += s;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    sRet += "xxx";
 }
 }
@@ -151,7 +151,7 @@ void f()
    sRet += ";";
 #endif
    sRet += " ";
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    sRet += side_effect();
 }
 }
@@ -172,9 +172,9 @@ namespace test6
 void f(OUString sComma, OUString maExtension, int mnDocumentIconID)
 {
    OUString sValue;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    sValue += sComma + sComma + maExtension + sComma;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    sValue += OUString::number(mnDocumentIconID) + sComma;
 }
 struct Foo
@@ -188,7 +188,7 @@ void g(int x, const Foo& aValidation)
    {
        case 1:
            sCondition += "cell-content-is-in-list(";
-            // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+            // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
            sCondition += aValidation.sFormula1 + ")";
    }
 }
@@ -251,7 +251,7 @@ C getC();
 void f1(C c)
 {
    OString s;
-    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
    s += c.constStringFunction(c.constIntFunction());
    s += c.constStringFunction(c.nonConstIntFunction());
    s += c.nonConstStringFunction();
@@ -259,4 +259,42 @@ void f1(C c)
 }
 }

+namespace test11
+{
+void f1()
+{
+    OUStringBuffer aFirstStr1("aaa");
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
+    aFirstStr1.append("...");
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
+    aFirstStr1.append("...");
+}
+}
+
+namespace test12
+{
+void f1(int j)
+{
+    OUStringBuffer aFirstStr1(12);
+    // no warning expected
+    aFirstStr1.append("...");
+    // expected-error@+1 {{simplify by merging with the preceding assign/append [loplugin:stringadd]}}
+    aFirstStr1.append("...");
+    // no warning expected
+    aFirstStr1.append(((j + 1) % 15) ? " " : "\n");
+}
+}
+
+namespace test13
+{
+void f1()
+{
+    OUStringBuffer aFirstStr1(12);
+    // no warning expected
+    aFirstStr1.append("...");
+    // because we have a comment between them
+    aFirstStr1.append("...");
+}
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
@@ -104,8 +104,7 @@ void handleCommand(
 #endif
        auto const env = getenv("SRC_ROOT");
        assert(env != nullptr);
-        buf.append(env);
-        buf.append("/solenv/bin/");
+        buf.append(OString::Concat(env) + "/solenv/bin/");
    }
    else
    {
@@ -116,19 +115,14 @@ void handleCommand(
 #endif
        auto const env = getenv("WORKDIR_FOR_BUILD");
        assert(env != nullptr);
-        buf.append(env);
-        buf.append("/LinkTarget/Executable/");
+        buf.append(OString::Concat(env) + "/LinkTarget/Executable/");
    }
-    buf.append(rExecutable.data());
-    buf.append(" -i ");
-    buf.append(rInPath);
-    buf.append(" -o ");
-    buf.append(rOutPath);
+    buf.append(OString::Concat(std::string_view(rExecutable))
+        + " -i " + rInPath + " -o " + rOutPath);

-    const OString cmd = buf.makeStringAndClear();
-    if (system(cmd.getStr()) != 0)
+    if (system(buf.getStr()) != 0)
    {
-        std::cerr << "Error: Failed to execute " << cmd << '\n';
+        std::cerr << "Error: Failed to execute " << buf.getStr() << '\n';
        throw false; //TODO
    }
 }
@@ -3113,9 +3113,8 @@ bool PDFWriterImpl::emitFonts()
                sal_uInt64 nStartPos = 0;
                if( aSubsetInfo.m_nFontType == FontType::SFNT_TTF )
                {
-                    aLine.append( static_cast<sal_Int32>(aBuffer.size()) );
-
-                    aLine.append( ">>\n"
+                    aLine.append( OString::number(static_cast<sal_Int32>(aBuffer.size()))
+                               + ">>\n"
                                 "stream\n" );
                    if ( !writeBuffer( aLine ) ) return false;
                    if ( osl::File::E_None != m_aFile.getPos(nStartPos) ) return false;
@@ -3138,14 +3137,13 @@ bool PDFWriterImpl::emitFonts()
                    getPfbSegmentLengths(aBuffer.data(), static_cast<int>(aBuffer.size()), aSegmentLengths);
                    // the lengths below are mandatory for PDF-exported Type1 fonts
                    // because the PFB segment headers get stripped! WhyOhWhy.
-                    aLine.append( static_cast<sal_Int32>(aSegmentLengths[0]) );
-                    aLine.append( "/Length2 " );
-                    aLine.append( static_cast<sal_Int32>(aSegmentLengths[1]) );
-                    aLine.append( "/Length3 " );
-                    aLine.append( static_cast<sal_Int32>(aSegmentLengths[2]) );
-
-                    aLine.append( ">>\n"
-                                 "stream\n" );
+                    aLine.append( OString::number(static_cast<sal_Int32>(aSegmentLengths[0]) )
+                        + "/Length2 "
+                        + OString::number( static_cast<sal_Int32>(aSegmentLengths[1]) )
+                        + "/Length3 "
+                        + OString::number( static_cast<sal_Int32>(aSegmentLengths[2]) )
+                        + ">>\n"
+                          "stream\n" );
                    if ( !writeBuffer( aLine ) ) return false;
                    if ( osl::File::E_None != m_aFile.getPos(nStartPos) ) return false;

@@ -3207,14 +3205,14 @@ bool PDFWriterImpl::emitFonts()
                    aLine.append( ((i & 15) == 15) ? "\n" : " " );
                }
                aLine.append( "]\n"
-                             "/FontDescriptor " );
-                aLine.append( nFontDescriptor );
-                aLine.append( " 0 R\n" );
+                             "/FontDescriptor "
+                    + OString::number( nFontDescriptor )
+                    + " 0 R\n" );
                if( nToUnicodeStream )
                {
-                    aLine.append( "/ToUnicode " );
-                    aLine.append( nToUnicodeStream );
-                    aLine.append( " 0 R\n" );
+                    aLine.append( "/ToUnicode "
+                        + OString::number( nToUnicodeStream )
+                        + " 0 R\n" );
                }
                aLine.append( ">>\n"
                             "endobj\n\n" );
@@ -3225,15 +3223,14 @@ bool PDFWriterImpl::emitFonts()
            else
            {
                OStringBuffer aErrorComment( 256 );
-                aErrorComment.append( "CreateFontSubset failed for font \"" );
-                aErrorComment.append( OUStringToOString( pFace->GetFamilyName(), RTL_TEXTENCODING_UTF8 ) );
-                aErrorComment.append( '\"' );
+                aErrorComment.append( "CreateFontSubset failed for font \""
+                    + OUStringToOString( pFace->GetFamilyName(), RTL_TEXTENCODING_UTF8 )
+                    + "\"" );
                if( pFace->GetItalic() == ITALIC_NORMAL )
                    aErrorComment.append( " italic" );
                else if( pFace->GetItalic() == ITALIC_OBLIQUE )
                    aErrorComment.append( " oblique" );
-                aErrorComment.append( " weight=" );
-                aErrorComment.append( sal_Int32(pFace->GetWeight()) );
+                aErrorComment.append( " weight=" + OString::number( sal_Int32(pFace->GetWeight()) ) );
                emitComment( aErrorComment.getStr() );
            }
        }
@@ -3264,11 +3261,11 @@ bool PDFWriterImpl::emitFonts()
    int ni = 0;
    for (auto const& itemMap : aFontIDToObject)
    {
-        aFontDict.append( "/F" );
-        aFontDict.append( itemMap.first );
-        aFontDict.append( ' ' );
-        aFontDict.append( itemMap.second );
-        aFontDict.append( " 0 R" );
+        aFontDict.append( "/F"
+            + OString::number( itemMap.first )
+            + " "
+            + OString::number( itemMap.second )
+            + " 0 R" );
        if( ((++ni) & 7) == 0 )
            aFontDict.append( '\n' );
    }
@@ -3304,8 +3301,8 @@ sal_Int32 PDFWriterImpl::emitResources()
    sal_Int32 nResourceDict = getResourceDictObj();
    CHECK_RETURN( updateObject( nResourceDict ) );
    aLine.setLength( 0 );
-    aLine.append( nResourceDict );
-    aLine.append( " 0 obj\n" );
+    aLine.append( OString::number(nResourceDict)
+        + " 0 obj\n" );
    m_aGlobalResourceDict.append( aLine, getFontDictObject() );
    aLine.append( "endobj\n\n" );
    CHECK_RETURN( writeBuffer( aLine ) );
@@ -3396,23 +3393,22 @@ sal_Int32 PDFWriterImpl::emitOutline()
        OStringBuffer aLine( 1024 );

        CHECK_RETURN( updateObject( rItem.m_nObject ) );
-        aLine.append( rItem.m_nObject );
-        aLine.append( " 0 obj\n" );
-        aLine.append( "<<" );
+        aLine.append( OString::number(rItem.m_nObject)
+            + " 0 obj\n"
+              "<<" );
        // number of visible children (all levels)
        if( i > 0 || aCounts[0] > 0 )
        {
-            aLine.append( "/Count " );
-            aLine.append( aCounts[i] );
+            aLine.append( "/Count " + OString::number( aCounts[i] ) );
        }
        if( ! rItem.m_aChildren.empty() )
        {
            // children list: First, Last
-            aLine.append( "/First " );
-            aLine.append( m_aOutline[rItem.m_aChildren.front()].m_nObject );
-            aLine.append( " 0 R/Last " );
-            aLine.append( m_aOutline[rItem.m_aChildren.back()].m_nObject );
-            aLine.append( " 0 R\n" );
+            aLine.append( "/First "
+                + OString::number( m_aOutline[rItem.m_aChildren.front()].m_nObject )
+                + " 0 R/Last "
+                + OString::number( m_aOutline[rItem.m_aChildren.back()].m_nObject )
+                + " 0 R\n" );
        }
        if( i > 0 )
        {
@@ -3426,20 +3422,20 @@ sal_Int32 PDFWriterImpl::emitOutline()
                aLine.append( "/Dest" );
                appendDest( rItem.m_nDestID, aLine );
            }
-            aLine.append( "/Parent " );
-            aLine.append( rItem.m_nParentObject );
-            aLine.append( " 0 R" );
+            aLine.append( "/Parent "
+                + OString::number( rItem.m_nParentObject )
+                + " 0 R" );
            if( rItem.m_nPrevObject )
            {
-                aLine.append( "/Prev " );
-                aLine.append( rItem.m_nPrevObject );
-                aLine.append( " 0 R" );
+                aLine.append( "/Prev "
+                    + OString::number( rItem.m_nPrevObject )
+                    + " 0 R" );
            }
            if( rItem.m_nNextObject )
            {
-                aLine.append( "/Next " );
-                aLine.append( rItem.m_nNextObject );
-                aLine.append( " 0 R" );
+                aLine.append( "/Next "
+                    + OString::number( rItem.m_nNextObject )
+                    + " 0 R" );
            }
        }
        aLine.append( ">>\nendobj\n\n" );
@@ -3561,10 +3557,10 @@ bool PDFWriterImpl::emitScreenAnnotations()
            continue;

        // Annot dictionary.
-        aLine.append(rScreen.m_nObject);
-        aLine.append(" 0 obj\n");
-        aLine.append("<</Type/Annot");
-        aLine.append("/Subtype/Screen/Rect[");
+        aLine.append(OString::number(rScreen.m_nObject)
+            + " 0 obj\n"
+              "<</Type/Annot"
+              "/Subtype/Screen/Rect[");
        appendFixedInt(rScreen.m_aRect.Left(), aLine);
        aLine.append(' ');
        appendFixedInt(rScreen.m_aRect.Top(), aLine);
@@ -3575,9 +3571,9 @@ bool PDFWriterImpl::emitScreenAnnotations()
        aLine.append("]");

        // Action dictionary.
-        aLine.append("/A<</Type/Action /S/Rendition /AN ");
-        aLine.append(rScreen.m_nObject);
-        aLine.append(" 0 R ");
+        aLine.append("/A<</Type/Action /S/Rendition /AN "
+            + OString::number(rScreen.m_nObject)
+            + " 0 R ");

        // Rendition dictionary.
        aLine.append("/R<</Type/Rendition /S/MR ");
@@ -3622,27 +3618,27 @@ bool PDFWriterImpl::emitScreenAnnotations()
        // Alt text is a "Multi-language Text Array"
        aLine.append(" /Alt [ () ");
        appendUnicodeTextStringEncrypt(rScreen.m_AltText, rScreen.m_nObject, aLine);
-        aLine.append(" ] ");
-        aLine.append(">>");
+        aLine.append(" ] "
+                     ">>");

        // End Rendition dictionary by requesting play/pause/stop controls.
-        aLine.append("/P<</BE<</C true >>>>");
-        aLine.append(">>");
+        aLine.append("/P<</BE<</C true >>>>"
+                     ">>");

        // End Action dictionary.
        aLine.append("/OP 0 >>");

        if (0 < rScreen.m_nStructParent)
        {
-            aLine.append("\n/StructParent ");
-            aLine.append(rScreen.m_nStructParent);
-            aLine.append("\n");
+            aLine.append("\n/StructParent "
+                + OString::number(rScreen.m_nStructParent)
+                + "\n");
        }

        // End Annot dictionary.
-        aLine.append("/P ");
-        aLine.append(m_aPages[rScreen.m_nPage].m_nPageObject);
-        aLine.append(" 0 R\n>>\nendobj\n\n");
+        aLine.append("/P "
+            + OString::number(m_aPages[rScreen.m_nPage].m_nPageObject)
+            + " 0 R\n>>\nendobj\n\n");
        CHECK_RETURN(writeBuffer(aLine));
    }