Fix clang warnings

.ci/script.sh

@@ -30,13 +30,37 @@ changed_files="$(git diff --name-only "$TRAVIS_COMMIT_RANGE" | grep -E '\.([CcHh
 
 if [ -n "${changed_files}" ]; then
     echo "Running cppcheck on ${changed_files}"
+    # Set some configuration/define to speed up cppcheck by limiting
+    # the combinations of configurations it will check.
+    # We force the values set with add_definition() in kpvcrlib/CMakeLists.txt
+    # and some from crengine/include/crsetup.h.
     # shellcheck disable=SC2086
-    cppcheck -j 4 --error-exitcode=2 ${changed_files}
+    cppcheck -j 4 --error-exitcode=2 --language=c++ \
+        -DUSE_FONTCONFIG=0 \
+        -DUSE_FREETYPE=1 \
+        -DUSE_HARFBUZZ=1 \
+        -DUSE_FRIBIDI=1 \
+        -DUSE_LIBUNIBREAK=1 \
+        -DUSE_UTF8PROC=1 \
+        -DUSE_NANOSVG=1 \
+        -DALLOW_KERNING=1 \
+        -DCR3_PATCH=1 \
+        -DLINUX=1 \
+        -D_LINUX=1 \
+        -DCR_RENDER_32BPP_RGB_PXFMT \
+        -DCR_EMULATE_GETTEXT \
+        -DBUILD_LITE=0 \
+        -DLBOOK=0 \
+        -UANDROID \
+        -UCR_POCKETBOOK \
+        ${changed_files}
 
     # ignore header files in clang-tidy for now
     # @TODO rename to *.hpp (or *.hxx)?
     # see https://github.com/koreader/crengine/pull/130#issuecomment-373823848
     changed_files="$(git diff --name-only "$TRAVIS_COMMIT_RANGE" | grep -E '\.([Cc]|[c]pp)$')"
+    # To check them all, uncomment this:
+    # changed_files="$(find crengine/src | grep -E '\.([Cc]|[c]pp)$')"
     echo "Running clang-tidy on ${changed_files}"
     # shellcheck disable=SC2086
     clang-tidy ${changed_files} -- -Icrengine/include

crengine/include/lvfntman.h

@@ -656,7 +656,9 @@ class LBitmapFont : public LVBaseFont
     virtual void Clear() { if (m_font) lvfontClose( m_font ); m_font = NULL; }
     virtual bool IsNull() const { return m_font==NULL; }
     virtual bool operator ! () const { return IsNull(); }
-    virtual ~LBitmapFont() { Clear(); }
+    virtual ~LBitmapFont() {
+        Clear(); // NOLINT: Call to virtual function during destruction
+    }
 };
 #endif
 

crengine/include/lvpagesplitter.h

@@ -154,7 +154,7 @@ template <typename T, int RESIZE_MULT, int RESIZE_ADD> class CompactArray
     ~CompactArray()
     {
         if ( _data )
-            delete _data;
+            delete _data; // NOLINT(clang-analyzer-cplusplus.NewDelete)
     }
     void add( T item )
     {
@@ -366,14 +366,14 @@ class LVRendPageContext
 
     LVFootNote * curr_note;
 
-    LVFootNote * getOrCreateFootNote( lString16 id )
+    LVFootNoteRef getOrCreateFootNote( lString16 id )
     {
         LVFootNoteRef ref = footNotes.get(id);
         if ( ref.isNull() ) {
             ref = LVFootNoteRef( new LVFootNote( id ) );
             footNotes.set( id, ref );
         }
-        return ref.get();
+        return ref;
     }
 
     void split();

crengine/include/lvptrvec.h

@@ -121,7 +121,7 @@ class LVPtrVector
     /// removes item from vector by index
     T * remove( int pos )
     {
-        if (pos < 0 || (unsigned)pos > (unsigned)_count)
+        if (pos < 0 || (unsigned)pos >= (unsigned)_count)
             crFatalError();
         int i;
         T * item = _list[pos];

crengine/include/lvref.h

@@ -10,6 +10,12 @@
 
 */
 
+// Note: clang-tidy does not really handle well our reference
+// counting classes instances, and triggers a lot of
+// "Use of memory after it is freed" warnings.
+// We added a few "//NOLINT" below to the lines where these
+// warnings were triggered on.
+
 #ifndef __LVREF_H_INCLUDED__
 #define __LVREF_H_INCLUDED__
 
@@ -82,7 +88,7 @@ template <class T> class LVFastRef
     inline void Release()
     {
         if ( _ptr ) {
-            if ( _ptr->Release()==0 ) {
+            if ( _ptr->Release()==0 ) { // NOLINT(clang-analyzer-cplusplus.NewDelete)
                 delete _ptr;
             }
             _ptr=NULL;
@@ -111,7 +117,7 @@ template <class T> class LVFastRef
     {
         _ptr = ref._ptr;
         if ( _ptr )
-            _ptr->AddRef();
+            _ptr->AddRef(); // NOLINT(clang-analyzer-cplusplus.NewDelete)
     }
 
     /// Destructor.
@@ -135,7 +141,7 @@ template <class T> class LVFastRef
             Release();
         }
         if ( ref._ptr )
-            (_ptr = ref._ptr)->AddRef();
+            (_ptr = ref._ptr)->AddRef(); // NOLINT(clang-analyzer-cplusplus.NewDelete)
         return *this;
     }
 
@@ -160,7 +166,7 @@ template <class T> class LVFastRef
     /** Imitates usual pointer behavior. 
     Usual way to access object fields. 
      */
-    T * operator -> () const { return _ptr; }
+    T * operator -> () const { return _ptr; } // NOLINT(clang-analyzer-cplusplus.NewDelete)
 
     /// Dereferences pointer to object.
     /** Imitates usual pointer behavior. */
@@ -176,7 +182,7 @@ template <class T> class LVFastRef
     /** Usual way to get pointer value. 
     \return stored pointer to object.
      */
-    T * get() const { return _ptr; }
+    T * get() const { return _ptr; } // NOLINT(clang-analyzer-cplusplus.NewDelete)
 
     /// Checks whether pointer is NULL or not.
     /** \return true if pointer is NULL.
@@ -208,7 +214,7 @@ template <class T> class LVProtectedFastRef
     {
         T * res = NULL;
         if ( _ptr ) {
-            if ( _ptr->Release()==0 ) {
+            if ( _ptr->Release()==0 ) { // NOLINT(clang-analyzer-cplusplus.NewDelete)
                 res = _ptr;
             }
             _ptr=NULL;
@@ -240,7 +246,7 @@ template <class T> class LVProtectedFastRef
         REF_GUARD
         _ptr = ref._ptr;
         if ( _ptr )
-            _ptr->AddRef();
+            _ptr->AddRef(); // NOLINT(clang-analyzer-cplusplus.NewDelete)
     }
 
     /// Destructor.
@@ -317,7 +323,7 @@ template <class T> class LVProtectedFastRef
     /** Imitates usual pointer behavior.
     Usual way to access object fields.
      */
-    T * operator -> () const { return _ptr; }
+    T * operator -> () const { return _ptr; } // NOLINT(clang-analyzer-cplusplus.NewDelete)
 
     /// Dereferences pointer to object.
     /** Imitates usual pointer behavior. */
@@ -333,7 +339,7 @@ template <class T> class LVProtectedFastRef
     /** Usual way to get pointer value.
     \return stored pointer to object.
      */
-    T * get() const { return _ptr; }
+    T * get() const { return _ptr; } // NOLINT(clang-analyzer-cplusplus.NewDelete)
 
     /// Checks whether pointer is NULL or not.
     /** \return true if pointer is NULL.
@@ -364,11 +370,11 @@ template <class T> class LVRef
 private:
     ref_count_rec_t * _ptr;
     //========================================
-    ref_count_rec_t * AddRef() const { ++_ptr->_refcount; return _ptr; }
+    ref_count_rec_t * AddRef() const { ++_ptr->_refcount; return _ptr; } // NOLINT(clang-analyzer-cplusplus.NewDelete)
     //========================================
     void Release()
     { 
-        if (--_ptr->_refcount == 0)
+        if (--_ptr->_refcount == 0) // NOLINT(clang-analyzer-cplusplus.NewDelete)
         {
             if (_ptr != &ref_count_rec_t::null_ref)
             {
@@ -458,7 +464,7 @@ template <class T> class LVRef
         }
         else
         {
-            if (_ptr->_obj!=obj)
+            if (_ptr->_obj!=obj) // NOLINT(clang-analyzer-cplusplus.NewDelete)
             {
                 Release();
                 _ptr = new ref_count_rec_t(obj);
@@ -471,7 +477,7 @@ template <class T> class LVRef
     /** Imitates usual pointer behavior. 
         Usual way to access object fields. 
     */
-    T * operator -> () const { return reinterpret_cast<T*>(_ptr->_obj); }
+    T * operator -> () const { return reinterpret_cast<T*>(_ptr->_obj); } // NOLINT(clang-analyzer-cplusplus.NewDelete)
 
     /// Dereferences pointer to object.
     /** Imitates usual pointer behavior. */
@@ -492,13 +498,13 @@ template <class T> class LVRef
     /// Checks whether pointer is NULL or not.
     /** \return true if pointer is NULL.
         \sa isNull() */
-    bool operator ! () const { return !_ptr->_obj; }
+    bool operator ! () const { return !_ptr->_obj; } // NOLINT(clang-analyzer-cplusplus.NewDelete)
 
     /// Checks whether pointer is NULL or not.
     /** \return true if pointer is NULL. 
         \sa operator !()
     */
-    bool isNull() const { return _ptr->_obj == NULL; }
+    bool isNull() const { return _ptr->_obj == NULL; } // NOLINT(clang-analyzer-cplusplus.NewDelete)
 };
 
 template <typename T >

crengine/include/lvstream.h

@@ -109,7 +109,9 @@ class LVStreamBuffer : public LVRefCounter
     /// get buffer size
     virtual lvsize_t getSize() = 0;
     /// flush on destroy
-    virtual ~LVStreamBuffer() { close(); }
+    virtual ~LVStreamBuffer() {
+        close(); // NOLINT: Call to virtual function during destruction
+    }
     /// detach from stream, write changes if necessary
     virtual bool close() { return true; }
 };

crengine/include/lvstsheet.h

@@ -222,7 +222,7 @@ class LVCssSelector {
     LVCssSelector( LVCssSelector & v );
     LVCssSelector() : _id(0), _specificity(0), _pseudo_elem(0),  _next(NULL), _rules(NULL) { }
     LVCssSelector(int specificity) : _id(0), _specificity(specificity), _pseudo_elem(0), _next(NULL), _rules(NULL) { }
-    ~LVCssSelector() { if (_next) delete _next; if (_rules) delete _rules; }
+    ~LVCssSelector() { if (_next) delete _next; if (_rules) delete _rules; } // NOLINT(clang-analyzer-cplusplus.NewDelete)
     bool parse( const char * &str, lxmlDocBase * doc );
     lUInt16 getElementNameId() { return _id; }
     bool check( const ldomNode * node ) const;

crengine/include/lvtinydom.h

@@ -1466,7 +1466,7 @@ class ldomXPointer
     /// return document
 	inline ldomDocument * getDocument() { return _data->getDocument(); }
     /// returns node pointer
-    inline ldomNode * getNode() const { return _data->getNode(); }
+    inline ldomNode * getNode() const { return _data->getNode(); } // NOLINT(clang-analyzer-cplusplus.NewDelete)
 #if BUILD_LITE!=1
     /// return parent final node, if found
     ldomNode * getFinalNode() const;
@@ -1487,7 +1487,7 @@ class ldomXPointer
 	/// remove reference
 	~ldomXPointer()
 	{
-		if (_data->decRef() == 0)
+		if (_data->decRef() == 0) // NOLINT(clang-analyzer-cplusplus.NewDelete)
 			delete _data;
 	}
     /// copy constructor
@@ -1523,7 +1523,7 @@ class ldomXPointer
     /// returns true for NULL pointer
 	bool isNull() const
 	{
-        return !_data || _data->isNull();
+        return !_data || _data->isNull(); // NOLINT(clang-analyzer-cplusplus.NewDelete)
 	}
     /// returns true if object is pointer
 	bool isPointer() const
@@ -1646,7 +1646,7 @@ class ldomXPointerEx : public ldomXPointer
 			return *this;
 		if (_data->decRef() == 0)
 			delete _data;
-		_data = new XPointerData( *v._data );
+		_data = new XPointerData( *v._data ); // NOLINT(clang-analyzer-cplusplus.NewDelete)
         initIndex();
         return *this;
     }
@@ -2462,7 +2462,7 @@ class ldomDocument : public lxmlDocBase
     /// saves recent changes to mapped file
     virtual bool updateMap(LVDocViewCallback * progressCallback=NULL) {
         CRTimerUtil infinite;
-        return updateMap(infinite, progressCallback)!=CR_ERROR;
+        return updateMap(infinite, progressCallback)!=CR_ERROR; // NOLINT: Call to virtual function during destruction
     }
 #endif
 

crengine/include/rtfimp.h

@@ -251,6 +251,7 @@ class LVRtfValueStack
     {
         if ( sp>=MAX_PROP_STACK_SIZE ) {
             error = true;
+            delete newdest;
         } else {
 #ifdef LOG_RTF_PARSING
             CRLog::trace("Changing destination. Level=%d old=%08X new=%08X", sp, (unsigned)dest, (unsigned)newdest);

crengine/src/crgui.cpp

@@ -1250,7 +1250,7 @@ int CRMenu::getScrollHeight()
     int scrollHeight = 0;
     CRScrollSkinRef sskin = skin->getScrollSkin();
     if ( nItems > _pageItems || !sskin->getAutohide() ) {
-        nItems = _pageItems;
+        // nItems = _pageItems;
         scrollHeight = SCROLL_HEIGHT;
         if ( sskin->getMinSize().y>0 )
             scrollHeight = sskin->getMinSize().y;

crengine/src/cri18n.cpp

@@ -258,7 +258,7 @@ bool CRIniFileTranslator::open(const char * fileName)
             lString8 value( eqpos+1, (lvsize_t)(elp - eqpos - 1));
             _map.set(name, value);
         }
-        for ( p=elp; *elp && *elp!='\r' && *elp!='\n'; elp++)
+        for ( ; *elp && *elp!='\r' && *elp!='\n'; elp++)
             ;
         p = elp;
 		while ( *p=='\r' || *p=='\n' )

crengine/src/crskin.cpp

@@ -1283,9 +1283,9 @@ void CRToolBarSkin::drawToolBar( LVDrawBuf & buf, const lvRect & rect, bool enab
     rc.shrinkBy( _margins );
     int width = 0;
 	for ( int i=0; i<_buttons->length(); i++ ) {
-		int flags = enabled ? CRButtonSkin::ENABLED : 0;
-		if (i == selectedButton && enabled)
-			flags |= CRButtonSkin::SELECTED;
+		// int flags = enabled ? CRButtonSkin::ENABLED : 0;
+		// if (i == selectedButton && enabled)
+		// 	flags |= CRButtonSkin::SELECTED;
 		LVRef<CRButtonSkin> button = _buttons->get(i);
 		if (!button.isNull()) {
 			width += button->getMinSize().x;

crengine/src/crtxtenc.cpp

@@ -1902,29 +1902,29 @@ int AutodetectCodePageUtf( const unsigned char * buf, int buf_size, char * cp_na
 {
     // checking byte order signatures
     if ( buf[0]==0xEF && buf[1]==0xBB && buf[2]==0xBF ) {
-        strcpy( cp_name, "utf-8" );
-        strcpy( lang_name, "en" );
+        strcpy( cp_name, "utf-8" );     // NOLINT: strcpy is fine with hardcoded string with len < 32
+        strcpy( lang_name, "en" );      // NOLINT
         return 1;
     } else if ( buf[0]==0 && buf[1]==0 && buf[2]==0xFE && buf[3]==0xFF ) {
-        strcpy( cp_name, "utf-32be" );
-        strcpy( lang_name, "en" );
+        strcpy( cp_name, "utf-32be" ); // NOLINT
+        strcpy( lang_name, "en" );     // NOLINT
         return 1;
     } else if ( buf[0]==0xFE && buf[1]==0xFF ) {
-        strcpy( cp_name, "utf-16be" );
-        strcpy( lang_name, "en" );
+        strcpy( cp_name, "utf-16be" ); // NOLINT
+        strcpy( lang_name, "en" );     // NOLINT
         return 1;
     } else if ( buf[0]==0xFF && buf[1]==0xFE && buf[2]==0 && buf[3]==0 ) {
-        strcpy( cp_name, "utf-32le" );
-        strcpy( lang_name, "en" );
+        strcpy( cp_name, "utf-32le" ); // NOLINT
+        strcpy( lang_name, "en" );     // NOLINT
         return 1;
     } else if ( buf[0]==0xFF && buf[1]==0xFE ) {
-        strcpy( cp_name, "utf-16le" );
-        strcpy( lang_name, "en" );
+        strcpy( cp_name, "utf-16le" ); // NOLINT
+        strcpy( lang_name, "en" );     // NOLINT
         return 1;
     }
     if ( isValidUtf8Data( buf, buf_size ) ) {
-        strcpy( cp_name, "utf-8" );
-        strcpy( lang_name, "en" );
+        strcpy( cp_name, "utf-8" );    // NOLINT
+        strcpy( lang_name, "en" );     // NOLINT
         return 1;
     }
    return 0;
@@ -2042,8 +2042,8 @@ int AutodetectCodePage(const unsigned char * buf, int buf_size, char * cp_name,
 		   bestq = q;
 	   }
    }
-   strcpy(cp_name, cp_stat_table[bestn].cp_name);
-   strcpy(lang_name, cp_stat_table[bestn].lang_name);
+   strcpy(cp_name, cp_stat_table[bestn].cp_name);     // NOLINT: strcpy is fine, all strings are len < 32
+   strcpy(lang_name, cp_stat_table[bestn].lang_name); // NOLINT
    CRLog::debug("Detected codepage:%s lang:%s index:%d %s", cp_name, lang_name, bestn, skipHtml ? "(skipHtml)" : "");
    if (skipHtml) {
        if (detectXmlHtmlEncoding(buf, buf_size, cp_name)) {