loplugin:unusedfields - look for fields that can be const, in comphelper

idea from tml.

Extend the unusedfields plugin to find fields that are only assigned in
the constructor.

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

comphelper/qa/container/comphelper_ifcontainer.cxx

@@ -42,7 +42,7 @@ struct ContainerStats {
 
 class ContainerListener : public cppu::WeakImplHelper< XEventListener >
 {
-    ContainerStats *m_pStats;
+    ContainerStats * const m_pStats;
 public:
     explicit ContainerListener(ContainerStats *pStats)
         : m_pStats(pStats) { m_pStats->m_nAlive++; }

comphelper/source/container/interfacecontainer2.cxx

@@ -39,13 +39,13 @@ namespace comphelper
 {
 
 OInterfaceIteratorHelper2::OInterfaceIteratorHelper2( OInterfaceContainerHelper2 & rCont_ )
-    : rCont( rCont_ )
+    : rCont( rCont_ ),
+      bIsList( rCont_.bIsList )
 {
     MutexGuard aGuard( rCont.rMutex );
     if( rCont.bInUse )
         // worst case, two iterators at the same time
         rCont.copyAndResetInUse();
-    bIsList = rCont_.bIsList;
     aData = rCont_.aData;
     if( bIsList )
     {

comphelper/source/eventattachermgr/eventattachermgr.cxx

@@ -134,8 +134,8 @@ private:
 class AttacherAllListener_Impl : public WeakImplHelper< XAllListener >
 {
     rtl::Reference<ImplEventAttacherManager> mxManager;
-    OUString                            aScriptType;
-    OUString                            aScriptCode;
+    OUString const                           aScriptType;
+    OUString const                           aScriptCode;
 
     /// @throws CannotConvertException
     void convertToEventReturn( Any & rRet, const Type & rRetType );

comphelper/source/misc/backupfilehelper.cxx

@@ -549,7 +549,7 @@ namespace
     {
     private:
         ExtensionInfoEntryVector    maEntries;
-        OUString maRegPath;
+        OUString const maRegPath;
 
     public:
         ExtensionInfo()
@@ -998,7 +998,7 @@ namespace
         sal_uInt32          mnOffset;           // offset in File (zero identifies new file)
         sal_uInt32          mnCrc32;            // checksum
         FileSharedPtr       maFile;             // file where to find the data (at offset)
-        bool                mbDoCompress;       // flag if this file is scheduled to be compressed when written
+        bool const          mbDoCompress;       // flag if this file is scheduled to be compressed when written
 
         bool copy_content_straight(oslFileHandle& rTargetHandle)
         {

comphelper/source/misc/hash.cxx

@@ -67,7 +67,7 @@ struct HashImpl
     }
 #endif
 
-    HashType meType;
+    HashType const meType;
 
     HashImpl(HashType eType):
         meType(eType)

comphelper/source/misc/profilezone.cxx

@@ -92,9 +92,9 @@ css::uno::Sequence<OUString> getRecordingAndClear()
 
 
 ProfileZone::ProfileZone(const char * sProfileId) :
-    m_sProfileId(sProfileId)
+    m_sProfileId(sProfileId),
+    m_aCreateTime(ProfileRecording::addRecording(sProfileId, 0))
 {
-    m_aCreateTime = ProfileRecording::addRecording(sProfileId, 0);
 }
 
 ProfileZone::~ProfileZone()

comphelper/source/officeinstdir/officeinstallationdirectories.hxx

@@ -69,8 +69,8 @@ public:
 private:
     void initDirs();
 
-    OUString                                   m_aOfficeBrandDirMacro;
-    OUString                                   m_aUserDirMacro;
+    OUString const                             m_aOfficeBrandDirMacro;
+    OUString const                             m_aUserDirMacro;
     css::uno::Reference< css::uno::XComponentContext >    m_xCtx;
     std::unique_ptr<OUString>                  m_pOfficeBrandDir;
     std::unique_ptr<OUString>                  m_pUserDir;

comphelper/source/property/propertycontainerhelper.cxx

@@ -51,7 +51,7 @@ namespace
     // comparing two property descriptions (by name)
     struct PropertyDescriptionNameMatch
     {
-        OUString m_rCompare;
+        OUString const m_rCompare;
         explicit PropertyDescriptionNameMatch( const OUString& _rCompare ) : m_rCompare( _rCompare ) { }
 
         bool operator() (const PropertyDescription& x ) const

comphelper/source/xml/ofopxmlhelper.cxx

@@ -45,23 +45,23 @@ namespace comphelper {
 class OFOPXMLHelper_Impl
     : public cppu::WeakImplHelper< css::xml::sax::XDocumentHandler >
 {
-    sal_uInt16 m_nFormat; // which format to parse
+    sal_uInt16 const m_nFormat; // which format to parse
 
     // Relations info related strings
-    OUString m_aRelListElement;
-    OUString m_aRelElement;
-    OUString m_aIDAttr;
-    OUString m_aTypeAttr;
-    OUString m_aTargetModeAttr;
-    OUString m_aTargetAttr;
+    OUString const m_aRelListElement;
+    OUString const m_aRelElement;
+    OUString const m_aIDAttr;
+    OUString const m_aTypeAttr;
+    OUString const m_aTargetModeAttr;
+    OUString const m_aTargetAttr;
 
     // ContentType related strings
-    OUString m_aTypesElement;
-    OUString m_aDefaultElement;
-    OUString m_aOverrideElement;
-    OUString m_aExtensionAttr;
-    OUString m_aPartNameAttr;
-    OUString m_aContentTypeAttr;
+    OUString const m_aTypesElement;
+    OUString const m_aDefaultElement;
+    OUString const m_aOverrideElement;
+    OUString const m_aExtensionAttr;
+    OUString const m_aPartNameAttr;
+    OUString const m_aContentTypeAttr;
 
     css::uno::Sequence< css::uno::Sequence< css::beans::StringPair > > m_aResultSeq;
     std::vector< OUString > m_aElementsSeq; // stack of elements being parsed

compilerplugins/clang/unusedfields.cxx

@@ -70,6 +70,7 @@ static std::set<MyFieldInfo> touchedFromOutsideSet;
 static std::set<MyFieldInfo> touchedFromOutsideConstructorSet;
 static std::set<MyFieldInfo> readFromSet;
 static std::set<MyFieldInfo> writeToSet;
+static std::set<MyFieldInfo> writeToOutsideConstructorSet;
 static std::set<MyFieldInfo> definitionSet;
 
 /**
@@ -159,6 +160,7 @@ public:
 private:
     MyFieldInfo niceName(const FieldDecl*);
     void checkTouchedFromOutside(const FieldDecl* fieldDecl, const Expr* memberExpr);
+    void checkWriteFromOutsideConstructor(const FieldDecl* fieldDecl, const Expr* memberExpr);
     void checkWriteOnly(const FieldDecl* fieldDecl, const Expr* memberExpr);
     void checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberExpr);
     bool isSomeKindOfZero(const Expr* arg);
@@ -193,6 +195,8 @@ void UnusedFields::run()
             output += "read:\t" + s.parentClass + "\t" + s.fieldName + "\n";
         for (const MyFieldInfo & s : writeToSet)
             output += "write:\t" + s.parentClass + "\t" + s.fieldName + "\n";
+        for (const MyFieldInfo & s : writeToOutsideConstructorSet)
+            output += "write-outside-constructor:\t" + s.parentClass + "\t" + s.fieldName + "\n";
         for (const MyFieldInfo & s : definitionSet)
             output += "definition:\t" + s.access + "\t" + s.parentClass + "\t" + s.fieldName + "\t" + s.fieldType + "\t" + s.sourceLocation + "\n";
         std::ofstream myfile;
@@ -671,7 +675,11 @@ void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberE
         RecordDecl const * cxxRecordDecl1 = fieldDecl->getParent();
         // we don't care about writes to a field when inside the copy/move constructor/operator= for that field
         if (cxxRecordDecl1 && (cxxRecordDecl1 == insideMoveOrCopyOrCloneDeclParent))
+        {
+            // ... but they matter to tbe can-be-const analysis
+            checkWriteFromOutsideConstructor(fieldDecl, memberExpr);
             return;
+        }
     }
 
     // if we're inside a block that looks like
@@ -881,7 +889,10 @@ void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberE
 
     MyFieldInfo fieldInfo = niceName(fieldDecl);
     if (bPotentiallyWrittenTo)
+    {
         writeToSet.insert(fieldInfo);
+        checkWriteFromOutsideConstructor(fieldDecl, memberExpr);
+    }
 }
 
 bool UnusedFields::IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt * child, CallerWrapper callExpr,
@@ -1010,6 +1021,27 @@ void UnusedFields::checkTouchedFromOutside(const FieldDecl* fieldDecl, const Exp
     }
 }
 
+// For the const-field analysis.
+// Called when we have a write to a field, and we want to record that write only if it's writing from
+// outside the constructor.
+void UnusedFields::checkWriteFromOutsideConstructor(const FieldDecl* fieldDecl, const Expr* memberExpr) {
+    const FunctionDecl* memberExprParentFunction = getParentFunctionDecl(memberExpr);
+    bool doWrite = false;
+
+    if (!memberExprParentFunction)
+        // If we are not inside a function
+        doWrite = true;
+    else if (memberExprParentFunction->getParent() != fieldDecl->getParent())
+        // or we are inside a method from another class (than the one the field belongs to)
+        doWrite = true;
+    else if (!isa<CXXConstructorDecl>(memberExprParentFunction))
+        // or we are not inside constructor
+        doWrite = true;
+
+    if (doWrite)
+        writeToOutsideConstructorSet.insert(niceName(fieldDecl));
+}
+
 llvm::Optional<CalleeWrapper> UnusedFields::getCallee(CallExpr const * callExpr)
 {
     FunctionDecl const * functionDecl = callExpr->getDirectCallee();

compilerplugins/clang/unusedfields.py

@@ -13,6 +13,7 @@ touchedFromOutsideSet = set()
 touchedFromOutsideConstructorSet = set()
 readFromSet = set()
 writeToSet = set()
+writeFromOutsideConstructorSet = set()
 sourceLocationSet = set()
 
 # clang does not always use exactly the same numbers in the type-parameter vars it generates
@@ -55,6 +56,8 @@ with io.open("workdir/loplugin.unusedfields.log", "rb", buffering=1024*1024) as
             readFromSet.add(parseFieldInfo(tokens))
         elif tokens[0] == "write:":
             writeToSet.add(parseFieldInfo(tokens))
+        elif tokens[0] == "write-outside-constructor:":
+            writeFromOutsideConstructorSet.add(parseFieldInfo(tokens))
         else:
             print( "unknown line: " + line)
 
@@ -223,6 +226,29 @@ for d in protectedAndPublicDefinitionSet:
 
     canBePrivateSet.add((clazz + " " + definitionToTypeMap[d], srcLoc))
 
+# Calculate can-be-const-field set
+canBeConstFieldSet = set()
+for d in definitionSet:
+    if d in writeFromOutsideConstructorSet:
+        continue
+    srcLoc = definitionToSourceLocationMap[d];
+    fieldType = definitionToTypeMap[d]
+    if fieldType.startswith("const "):
+        continue
+    if "std::unique_ptr" in fieldType:
+        continue
+    if "std::shared_ptr" in fieldType:
+        continue
+    if "Reference<" in fieldType:
+        continue
+    if "VclPtr<" in fieldType:
+        continue
+    if "osl::Mutex" in fieldType:
+        continue
+    if "::sfx2::sidebar::ControllerItem" in fieldType:
+        continue
+    canBeConstFieldSet.add((d[0] + " " + d[1] + " " + fieldType, srcLoc))
+
 
 # sort the results using a "natural order" so sequences like [item1,item2,item10] sort nicely
 def natural_sort_key(s, _nsre=re.compile('([0-9]+)')):
@@ -235,6 +261,7 @@ tmp2list = sorted(writeonlySet, key=lambda v: natural_sort_key(v[1]))
 tmp3list = sorted(canBePrivateSet, key=lambda v: natural_sort_key(v[1]))
 tmp4list = sorted(readonlySet, key=lambda v: natural_sort_key(v[1]))
 tmp5list = sorted(onlyUsedInConstructorSet, key=lambda v: natural_sort_key(v[1]))
+tmp6list = sorted(canBeConstFieldSet, key=lambda v: natural_sort_key(v[1]))
 
 # print out the results
 with open("compilerplugins/clang/unusedfields.untouched.results", "wt") as f:
@@ -258,5 +285,9 @@ with open("compilerplugins/clang/unusedfields.only-used-in-constructor.results",
     for t in tmp5list:
         f.write( t[1] + "\n" )
         f.write( "    " + t[0] + "\n" )
+with open("compilerplugins/clang/unusedfields.can-be-const.results", "wt") as f:
+    for t in tmp6list:
+        f.write( t[1] + "\n" )
+        f.write( "    " + t[0] + "\n" )
 
 

include/comphelper/ChainablePropertySet.hxx

@@ -68,7 +68,7 @@ namespace comphelper
     {
         friend class MasterPropertySet;
     protected:
-        SolarMutex* mpMutex;
+        SolarMutex* const mpMutex;
         rtl::Reference < ChainablePropertySetInfo > mxInfo;
 
         /// @throws css::beans::UnknownPropertyException

include/comphelper/MasterPropertySet.hxx

@@ -60,7 +60,7 @@ namespace comphelper
                               public css::beans::XMultiPropertySet
     {
     protected:
-        SolarMutex* mpMutex;
+        SolarMutex* const mpMutex;
         sal_uInt8 mnLastId;
         std::map< sal_uInt8, comphelper::SlaveData* >  maSlaveMap;
         rtl::Reference< MasterPropertySetInfo >        mxInfo;

include/comphelper/PropertyInfoHash.hxx

@@ -28,14 +28,14 @@ namespace comphelper
 {
     struct PropertyInfo
     {
-        OUString maName;
-        sal_Int32 mnHandle;
-        css::uno::Type maType;
-        sal_Int16 mnAttributes;
+        OUString const maName;
+        sal_Int32 const mnHandle;
+        css::uno::Type const maType;
+        sal_Int16 const mnAttributes;
     };
     struct PropertyData
     {
-        sal_uInt8 mnMapId;
+        sal_uInt8 const mnMapId;
         PropertyInfo const *mpInfo;
         PropertyData ( sal_uInt8 nMapId, PropertyInfo const *pInfo )
         : mnMapId ( nMapId )

include/comphelper/asyncnotification.hxx

@@ -204,7 +204,7 @@ namespace comphelper
         typedef EVENT_OBJECT    EventObjectType;
 
     private:
-        EventObjectType m_aEvent;
+        EventObjectType const m_aEvent;
 
     public:
         EventHolder( const EventObjectType& _rEvent )

include/comphelper/componentmodule.hxx

@@ -53,13 +53,13 @@ namespace comphelper
     struct COMPHELPER_DLLPUBLIC ComponentDescription
     {
         /// the implementation name of the component
-        OUString                                     sImplementationName;
+        OUString const                               sImplementationName;
         /// the services supported by the component implementation
-        css::uno::Sequence< OUString >               aSupportedServices;
+        css::uno::Sequence< OUString > const         aSupportedServices;
         /// the function to create an instance of the component
-        ::cppu::ComponentFactoryFunc                 pComponentCreationFunc;
+        ::cppu::ComponentFactoryFunc const           pComponentCreationFunc;
         /// the function to create a factory for the component (usually <code>::cppu::createSingleComponentFactory</code>)
-        FactoryInstantiation                         pFactoryCreationFunc;
+        FactoryInstantiation const                   pFactoryCreationFunc;
 
         ComponentDescription(
                 const OUString& _rImplementationName,

include/comphelper/enumhelper.hxx

@@ -45,7 +45,7 @@ class COMPHELPER_DLLPUBLIC OEnumerationByName : private OEnumerationLock
                          , public ::cppu::WeakImplHelper< css::container::XEnumeration ,
                                                           css::lang::XEventListener    >
 {
-    css::uno::Sequence< OUString >                m_aNames;
+    css::uno::Sequence< OUString > const                m_aNames;
     sal_Int32                                           m_nPos;
     css::uno::Reference< css::container::XNameAccess >    m_xAccess;
     bool                                            m_bListening;

include/comphelper/flagguard.hxx

@@ -45,7 +45,7 @@ namespace comphelper
         struct RestoreFlag
         {
             bool & rFlag;
-            bool originalValue;
+            bool const originalValue;
             RestoreFlag(bool & i_flagRef)
                 : rFlag(i_flagRef), originalValue(i_flagRef) {}
             void operator()()

include/comphelper/interaction.hxx

@@ -112,7 +112,7 @@ namespace comphelper
     */
     class COMPHELPER_DLLPUBLIC OInteractionRequest : public OInteractionRequest_Base
     {
-        css::uno::Any
+        css::uno::Any const
                     m_aRequest;         /// the request we represent
         std::vector< css::uno::Reference< css::task::XInteractionContinuation > >
                     m_aContinuations;   /// all registered continuations

include/comphelper/interfacecontainer2.hxx

@@ -105,7 +105,7 @@ public:
 
 private:
     OInterfaceContainerHelper2 & rCont;
-    bool                         bIsList;
+    bool const                   bIsList;
     detail::element_alias2       aData;
     sal_Int32                    nRemain;
 
@@ -262,8 +262,8 @@ private:
     {
     private:
         typedef void ( SAL_CALL ListenerT::*NotificationMethod )( const EventT& );
-        NotificationMethod  m_pMethod;
-        const EventT&       m_rEvent;
+        NotificationMethod const m_pMethod;
+        const EventT&            m_rEvent;
     public:
         NotifySingleListener( NotificationMethod method, const EventT& event ) : m_pMethod( method ), m_rEvent( event ) { }
 

include/comphelper/profilezone.hxx

@@ -36,7 +36,7 @@ class COMPHELPER_DLLPUBLIC ProfileZone
 {
 private:
     const char * m_sProfileId;
-    long long m_aCreateTime;
+    long long const m_aCreateTime;
 public:
 
     // Note that the char pointer is stored as such in the ProfileZone object and used in the

include/comphelper/propmultiplex.hxx

@@ -82,7 +82,7 @@ namespace comphelper
         OPropertyChangeListener*                        m_pListener;
         sal_Int32                                       m_nLockCount;
         bool                                            m_bListening        : 1;
-        bool                                            m_bAutoSetRelease   : 1;
+        bool const                                      m_bAutoSetRelease   : 1;
 
 
         virtual ~OPropertyChangeMultiplexer() override;

include/comphelper/seqstream.hxx

@@ -39,7 +39,7 @@ class COMPHELPER_DLLPUBLIC SequenceInputStream
 : public ::cppu::WeakImplHelper< css::io::XInputStream, css::io::XSeekable >
 {
     ::osl::Mutex    m_aMutex;
-    css::uno::Sequence<sal_Int8> m_aData;
+    css::uno::Sequence<sal_Int8> const m_aData;
     sal_Int32       m_nPos;
 
 public:
@@ -75,7 +75,7 @@ private:
     void finalizeOutput();
     css::uno::Sequence< sal_Int8 >&                 m_rSequence;
     double                                          m_nResizeFactor;
-    sal_Int32                                       m_nMinimumResize;
+    sal_Int32 const                                 m_nMinimumResize;
     sal_Int32                                       m_nSize;
         // the size of the virtual stream. This is not the size of the sequence, but the number of bytes written
         // into the stream at a given moment.

include/comphelper/stl_types.hxx

@@ -52,7 +52,7 @@ public:
 
 class UStringMixEqual
 {
-    bool m_bCaseSensitive;
+    bool const m_bCaseSensitive;
 
 public:
     UStringMixEqual(bool bCaseSensitive = true):m_bCaseSensitive(bCaseSensitive){}
@@ -121,7 +121,7 @@ public:
     explicit mem_fun1_t(_fun_type pf) : M_f(pf) {}
     void operator()(Tp* p, Arg x) const { (p->*M_f)(x); }
 private:
-    _fun_type M_f;
+    _fun_type const M_f;
 };
 
 template <class Tp, class Arg>

include/comphelper/string.hxx

@@ -339,7 +339,7 @@ COMPHELPER_DLLPUBLIC sal_Int32 compareNatural( const OUString &rLHS, const OUStr
 class COMPHELPER_DLLPUBLIC NaturalStringSorter
 {
 private:
-    css::lang::Locale                                m_aLocale;
+    css::lang::Locale const                          m_aLocale;
     css::uno::Reference< css::i18n::XCollator >      m_xCollator;
     css::uno::Reference< css::i18n::XBreakIterator > m_xBI;
 public:

include/comphelper/threadpool.hxx

@@ -92,7 +92,7 @@ private:
     std::mutex              maMutex;
     std::condition_variable maTasksChanged;
     bool                    mbTerminate;
-    std::size_t             mnWorkers;
+    std::size_t const       mnWorkers;
     std::vector< std::unique_ptr<ThreadTask> >   maTasks;
     std::vector< rtl::Reference< ThreadWorker > > maWorkers;
 };

include/comphelper/unique_disposing_ptr.hxx

@@ -74,7 +74,7 @@ private:
     private:
         css::uno::Reference< css::lang::XComponent > m_xComponent;
         unique_disposing_ptr<T>& m_rItem;
-        bool mbComponentDLL;
+        bool const mbComponentDLL;
     public:
         TerminateListener(const css::uno::Reference< css::lang::XComponent > &rComponent,
             unique_disposing_ptr<T>& rItem, bool bComponentDLL) :