new loplugin:sequenceloop

look for places we should be using std::as_const on for-range loops over uno::Sequence, to avoid triggering a copy Change-Id: I7efb641bf09d37c87946f03428ee4eec90298c8a Reviewed-on: https://gerrit.libreoffice.org/77441 Tested-by: Jenkins Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
darshan-yadav · Aug 16, 2019 · 7d7a786 · 7d7a786
1 parent abeab40
commit 7d7a786
Show file tree

Hide file tree

Showing 17 changed files with 138 additions and 25 deletions.
diff --git a/compilerplugins/clang/sequenceloop.cxx b/compilerplugins/clang/sequenceloop.cxx
@@ -0,0 +1,76 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <cassert>
+#include <string>
+#include <iostream>
+#include <fstream>
+#include <set>
+
+#include <clang/AST/CXXInheritance.h>
+#include "plugin.hxx"
+#include "check.hxx"
+
+/**
+  When used in "for" loops, css::uno::Sequence objects tend to end up calling the non-const begin()/end(),
+  which is considerably more expensive than the const variants because it forces a local copy
+  of the internal ref-counted impl object.
+*/
+
+namespace
+{
+class SequenceLoop : public loplugin::FilteringPlugin<SequenceLoop>
+{
+public:
+    explicit SequenceLoop(loplugin::InstantiationData const& data)
+        : FilteringPlugin(data)
+    {
+    }
+
+    virtual void run() override
+    {
+        if (preRun())
+            TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+    }
+
+    bool VisitCXXForRangeStmt(CXXForRangeStmt const*);
+};
+
+bool SequenceLoop::VisitCXXForRangeStmt(CXXForRangeStmt const* forStmt)
+{
+    if (ignoreLocation(forStmt))
+        return true;
+
+    auto tc = loplugin::TypeCheck(forStmt->getRangeInit()->getType());
+    if (tc.Const())
+        return true;
+    if (!tc.Class("Sequence")
+             .Namespace("uno")
+             .Namespace("star")
+             .Namespace("sun")
+             .Namespace("com")
+             .GlobalNamespace())
+        return true;
+    const VarDecl* varDecl = forStmt->getLoopVariable();
+    auto tc2 = loplugin::TypeCheck(varDecl->getType());
+    if (!tc2.LvalueReference().Const())
+        return true;
+
+    report(DiagnosticsEngine::Warning,
+           "use std::as_const, or make range var const, to avoid creating a copy of the Sequence",
+           compat::getBeginLoc(forStmt))
+        << forStmt->getSourceRange();
+    return true;
+}
+
+loplugin::Plugin::Registration<SequenceLoop> X("sequenceloop");
+
+} // namespace
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/test/sequenceloop.cxx b/compilerplugins/clang/test/sequenceloop.cxx
@@ -0,0 +1,35 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <com/sun/star/uno/Sequence.hxx>
+#include <com/sun/star/uno/XInterface.hpp>
+#include <utility>
+
+namespace test1
+{
+void foo(css::uno::Sequence<css::uno::Reference<css::uno::XInterface>>& aSeq)
+{
+    // expected-error@+1 {{use std::as_const, or make range var const, to avoid creating a copy of the Sequence [loplugin:sequenceloop]}}
+    for (const auto& x : aSeq)
+        x.get();
+    // no warning expected
+    for (auto& x : aSeq)
+        x.get();
+    for (const auto& x : std::as_const(aSeq))
+        x.get();
+}
+// no warning expected
+void foo2(const css::uno::Sequence<css::uno::Reference<css::uno::XInterface>>& aSeq)
+{
+    for (const auto& x : aSeq)
+        x.get();
+}
+};
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/editeng/source/accessibility/AccessibleEditableTextPara.cxx b/editeng/source/accessibility/AccessibleEditableTextPara.cxx
@@ -2424,7 +2424,7 @@ namespace accessibility
         uno::Sequence< beans::PropertyValue > aOutSequence( aProperties.getLength() );
         beans::PropertyValue* pOutSequence = aOutSequence.getArray();
         sal_Int32 nOutLen = 0;
-        for (const beans::Property& rProperty : aProperties)
+        for (const beans::Property& rProperty : std::as_const(aProperties))
         {
             // calling implementation functions:
             // _getPropertyState and _getPropertyValue (see below) to provide
@@ -2510,7 +2510,7 @@ namespace accessibility
         uno::Sequence< beans::PropertyValue > aOutSequence( aProperties.getLength() );
         beans::PropertyValue* pOutSequence = aOutSequence.getArray();
         sal_Int32 nOutLen = 0;
-        for (const beans::Property& rProperty : aProperties)
+        for (const beans::Property& rProperty : std::as_const(aProperties))
         {
             // calling 'regular' functions that will operate on the selection
             PropertyState eState = xPropSet->getPropertyState( rProperty.Name );

diff --git a/filter/source/xsltdialog/xmlfiltersettingsdialog.cxx b/filter/source/xsltdialog/xmlfiltersettingsdialog.cxx
@@ -340,7 +340,7 @@ OUString XMLFilterSettingsDialog::createUniqueInterfaceName( const OUString& rIn
 
     try
     {
-        Sequence< OUString > aFilterNames( mxFilterContainer->getElementNames() );
+        const Sequence< OUString > aFilterNames( mxFilterContainer->getElementNames() );
 
         Sequence< PropertyValue > aValues;
         for( OUString const & filterName : aFilterNames)
@@ -960,7 +960,7 @@ void XMLFilterSettingsDialog::initFilterList()
 {
     if( mxFilterContainer.is() )
     {
-        Sequence< OUString > aFilterNames( mxFilterContainer->getElementNames() );
+        const Sequence< OUString > aFilterNames( mxFilterContainer->getElementNames() );
 
         Sequence< PropertyValue > aValues;
 

diff --git a/filter/source/xsltdialog/xmlfiltertestdialog.cxx b/filter/source/xsltdialog/xmlfiltertestdialog.cxx
@@ -290,7 +290,7 @@ void XMLFilterTestDialog::onExportBrowse()
         Reference< XNameAccess > xTypeDetection( mxContext->getServiceManager()->createInstanceWithContext( "com.sun.star.document.TypeDetection", mxContext ), UNO_QUERY );
         if( xFilterContainer.is() && xTypeDetection.is() )
         {
-            Sequence< OUString > aFilterNames( xFilterContainer->getElementNames() );
+            const Sequence< OUString > aFilterNames( xFilterContainer->getElementNames() );
 
             for( OUString const & filterName : aFilterNames )
             {
@@ -306,7 +306,7 @@ void XMLFilterTestDialog::onExportBrowse()
 
                 int nFound = 0;
 
-                for( const PropertyValue& rValue : aValues )
+                for( const PropertyValue& rValue : std::as_const(aValues) )
                 {
                     if ( rValue.Name == "Type" )
                     {

diff --git a/lingucomponent/source/spellcheck/spell/sspellimp.cxx b/lingucomponent/source/spellcheck/spell/sspellimp.cxx
@@ -124,7 +124,7 @@ Sequence< Locale > SAL_CALL SpellChecker::getLocales()
         uno::Sequence< OUString > aFormatList;
         aLinguCfg.GetSupportedDictionaryFormatsFor( "SpellCheckers",
                 "org.openoffice.lingu.MySpellSpellChecker", aFormatList );
-        for (auto const& format : aFormatList)
+        for (auto const& format : std::as_const(aFormatList))
         {
             std::vector< SvtLinguConfigDictionaryEntry > aTmpDic(
                     aLinguCfg.GetActiveDictionariesByFormat(format) );
@@ -150,7 +150,7 @@ Sequence< Locale > SAL_CALL SpellChecker::getLocales()
             std::set<OUString> aLocaleNamesSet;
             for (auto const& dict : aDics)
             {
-                uno::Sequence< OUString > aLocaleNames( dict.aLocaleNames );
+                const uno::Sequence< OUString > aLocaleNames( dict.aLocaleNames );
                 uno::Sequence< OUString > aLocations( dict.aLocations );
                 SAL_WARN_IF(
                     aLocaleNames.hasElements() && !aLocations.hasElements(),
@@ -200,7 +200,7 @@ Sequence< Locale > SAL_CALL SpellChecker::getLocales()
                 if (dict.aLocaleNames.hasElements() &&
                     dict.aLocations.hasElements())
                 {
-                    uno::Sequence< OUString > aLocaleNames( dict.aLocaleNames );
+                    const uno::Sequence< OUString > aLocaleNames( dict.aLocaleNames );
 
                     // currently only one language per dictionary is supported in the actual implementation...
                     // Thus here we work-around this by adding the same dictionary several times.
@@ -238,7 +238,7 @@ sal_Bool SAL_CALL SpellChecker::hasLocale(const Locale& rLocale)
     if (!m_aSuppLocales.hasElements())
         getLocales();
 
-    for (auto const& suppLocale : m_aSuppLocales)
+    for (auto const& suppLocale : std::as_const(m_aSuppLocales))
     {
         if (rLocale == suppLocale)
         {

diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -56,6 +56,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
     compilerplugins/clang/test/salunicodeliteral \
     compilerplugins/clang/test/selfinit \
     compilerplugins/clang/test/sequentialassign \
+    compilerplugins/clang/test/sequenceloop \
     compilerplugins/clang/test/shouldreturnbool \
     compilerplugins/clang/test/simplifybool \
     compilerplugins/clang/test/simplifyconstruct \

diff --git a/xmlscript/source/xmldlg_imexp/xmldlg_expmodels.cxx b/xmlscript/source/xmldlg_imexp/xmldlg_expmodels.cxx
@@ -318,7 +318,7 @@ void ElementDescriptor::readComboBoxModel( StyleBag * all_styles )
     {
         ElementDescriptor * popup = new ElementDescriptor( _xProps, _xPropState, XMLNS_DIALOGS_PREFIX ":menupopup", _xDocument );
 
-        for ( const auto& rItemValue : itemValues )
+        for ( const auto& rItemValue : std::as_const(itemValues) )
         {
             ElementDescriptor * item = new ElementDescriptor( _xProps, _xPropState, XMLNS_DIALOGS_PREFIX ":menuitem", _xDocument );
             item->addAttribute( XMLNS_DIALOGS_PREFIX ":value", rItemValue );
@@ -365,7 +365,7 @@ void ElementDescriptor::readListBoxModel( StyleBag * all_styles )
     {
         ElementDescriptor * popup = new ElementDescriptor( _xProps, _xPropState, XMLNS_DIALOGS_PREFIX ":menupopup", _xDocument );
 
-        for ( const auto& rItemValue : itemValues )
+        for ( const auto& rItemValue : std::as_const(itemValues) )
         {
             ElementDescriptor * item = new ElementDescriptor(_xProps, _xPropState, XMLNS_DIALOGS_PREFIX ":menuitem", _xDocument );
             item->addAttribute( XMLNS_DIALOGS_PREFIX ":value", rItemValue );
@@ -1086,7 +1086,7 @@ void ElementDescriptor::readBullitinBoard( StyleBag * all_styles )
     Reference<  container::XNameContainer > xDialogModel( _xProps, UNO_QUERY );
     if ( !xDialogModel.is() )
         return; // #TODO throw???
-    Sequence< OUString > aElements( xDialogModel->getElementNames() );
+    const Sequence< OUString > aElements( xDialogModel->getElementNames() );
 
     ElementDescriptor * pRadioGroup = nullptr;
 

diff --git a/xmlscript/source/xmldlg_imexp/xmldlg_export.cxx b/xmlscript/source/xmldlg_imexp/xmldlg_export.cxx
@@ -1151,7 +1151,7 @@ void ElementDescriptor::readEvents()
         Reference< container::XNameContainer > xEvents( xSupplier->getEvents() );
         if (xEvents.is())
         {
-            Sequence< OUString > aNames( xEvents->getElementNames() );
+            const Sequence< OUString > aNames( xEvents->getElementNames() );
             for ( const auto& rName : aNames )
             {
                 script::ScriptEventDescriptor descr;

diff --git a/xmlscript/source/xmlflat_imexp/xmlbas_export.cxx b/xmlscript/source/xmlflat_imexp/xmlbas_export.cxx
@@ -153,7 +153,7 @@ sal_Bool XMLBasicExporterBase::filter( const Sequence< beans::PropertyValue >& /
 
                 if ( xLibContainer.is() )
                 {
-                    Sequence< OUString > aLibNames = xLibContainer->getElementNames();
+                    const Sequence< OUString > aLibNames = xLibContainer->getElementNames();
                     for ( const OUString& rLibName : aLibNames )
                     {
                         if ( xLibContainer->hasByName( rLibName ) )
@@ -229,7 +229,7 @@ sal_Bool XMLBasicExporterBase::filter( const Sequence< beans::PropertyValue >& /
 
                                 if ( xLib.is() )
                                 {
-                                    Sequence< OUString > aModNames = xLib->getElementNames();
+                                    const Sequence< OUString > aModNames = xLib->getElementNames();
                                     for ( const OUString& rModName : aModNames )
                                     {
                                         if ( xLib->hasByName( rModName ) )

diff --git a/xmlsecurity/qa/unit/signing/signing.cxx b/xmlsecurity/qa/unit/signing/signing.cxx
@@ -190,7 +190,7 @@ SigningTest::getCertificate(DocumentSignatureManager& rSignatureManager,
 {
     uno::Reference<xml::crypto::XSecurityEnvironment> xSecurityEnvironment
         = rSignatureManager.getSecurityEnvironment();
-    uno::Sequence<uno::Reference<security::XCertificate>> aCertificates
+    const uno::Sequence<uno::Reference<security::XCertificate>> aCertificates
         = xSecurityEnvironment->getPersonalCertificates();
 
     for (const auto& xCertificate : aCertificates)

diff --git a/xmlsecurity/source/dialogs/certificatechooser.cxx b/xmlsecurity/source/dialogs/certificatechooser.cxx
@@ -198,7 +198,7 @@ void CertificateChooser::ImplInitialize()
 
 
         // fill list of certificates; the first entry will be selected
-        for ( const auto& xCert : xCerts )
+        for ( const auto& xCert : std::as_const(xCerts) )
         {
             std::shared_ptr<UserData> userData = std::make_shared<UserData>();
             userData->xCertificate = xCert;

diff --git a/xmlsecurity/source/dialogs/macrosecurity.cxx b/xmlsecurity/source/dialogs/macrosecurity.cxx
@@ -357,7 +357,7 @@ MacroSecurityTrustedSourcesTP::MacroSecurityTrustedSourcesTP(weld::Container* pP
 
     FillCertLB();
 
-    css::uno::Sequence< OUString > aSecureURLs = m_pDlg->m_aSecOptions.GetSecureURLs();
+    const css::uno::Sequence< OUString > aSecureURLs = m_pDlg->m_aSecOptions.GetSecureURLs();
     mbURLsReadonly = m_pDlg->m_aSecOptions.IsReadOnly( SvtSecurityOptions::EOption::SecureUrls );
     m_xTrustFileROFI->set_visible(mbURLsReadonly);
     m_xTrustFileLocLB->set_sensitive(!mbURLsReadonly);

diff --git a/xmlsecurity/source/helper/documentsignaturehelper.cxx b/xmlsecurity/source/helper/documentsignaturehelper.cxx
@@ -86,7 +86,7 @@ static void ImplFillElementList(
     const OUString& rRootStorageName, const bool bRecursive,
     const DocumentSignatureAlgorithm mode)
 {
-    Sequence< OUString > aElements = rxStore->getElementNames();
+    const Sequence< OUString > aElements = rxStore->getElementNames();
 
     for ( const auto& rName : aElements )
     {
@@ -213,7 +213,7 @@ DocumentSignatureHelper::CreateElementList(
                     xSubStore.clear();
 
                     // Object folders...
-                    Sequence< OUString > aElementNames = rxStore->getElementNames();
+                    const Sequence< OUString > aElementNames = rxStore->getElementNames();
                     for ( const auto& rName : aElementNames )
                     {
                         if ( ( rName.match( "Object " ) ) && rxStore->isStorageElement( rName ) )

diff --git a/xmlsecurity/source/helper/documentsignaturemanager.cxx b/xmlsecurity/source/helper/documentsignaturemanager.cxx
@@ -177,7 +177,7 @@ bool DocumentSignatureManager::isXML(const OUString& rURI)
 
     if (readManifest())
     {
-        for (const uno::Sequence<beans::PropertyValue>& entry : m_manifest)
+        for (const uno::Sequence<beans::PropertyValue>& entry : std::as_const(m_manifest))
         {
             OUString sPath;
             OUString sMediaType;
@@ -393,7 +393,7 @@ bool DocumentSignatureManager::add(
                                              eAlgorithmID);
     }
 
-    uno::Sequence<uno::Reference<security::XCertificate>> aCertPath
+    const uno::Sequence<uno::Reference<security::XCertificate>> aCertPath
         = xSecurityContext->getSecurityEnvironment()->buildCertificatePath(xCert);
 
     OUStringBuffer aStrBuffer;

diff --git a/xmlsecurity/source/helper/ooxmlsecexporter.cxx b/xmlsecurity/source/helper/ooxmlsecexporter.cxx
@@ -242,7 +242,7 @@ void OOXMLSecExporter::Impl::writeRelationshipTransform(const OUString& rURI)
         m_xDocumentHandler->startElement("Transform", uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
     }
 
-    uno::Sequence< uno::Sequence<beans::StringPair> > aRelationsInfo = comphelper::OFOPXMLHelper::ReadRelationsInfoSequence(xRelStream, rURI, m_xComponentContext);
+    const uno::Sequence< uno::Sequence<beans::StringPair> > aRelationsInfo = comphelper::OFOPXMLHelper::ReadRelationsInfoSequence(xRelStream, rURI, m_xComponentContext);
     for (const uno::Sequence<beans::StringPair>& rPairs : aRelationsInfo)
     {
         OUString aId;

diff --git a/xmlsecurity/source/xmlsec/nss/xmlsignature_nssimpl.cxx b/xmlsecurity/source/xmlsec/nss/xmlsignature_nssimpl.cxx
@@ -288,7 +288,8 @@ OUString SAL_CALL XMLSignature_NssImpl::getImplementationName()
 /* XServiceInfo */
 sal_Bool SAL_CALL XMLSignature_NssImpl::supportsService(const OUString& rServiceName)
 {
-    for (OUString const & rCurrentServiceName : getSupportedServiceNames())
+    const css::uno::Sequence<OUString> aServiceNames = getSupportedServiceNames();
+    for (OUString const & rCurrentServiceName : aServiceNames)
     {
         if (rCurrentServiceName == rServiceName)
             return true;