[vlc-devel] [PATCH 02/11] demux/mkv: cleaned `chapters.{cpp, hpp}`

[Denis Charmet]

Hi,

On 2016-03-02 18:04, Filip Roséen wrote:
> chapters.cpp
> ------------
> 
>   - `delete` is a NOOP if the operand is NULL, if-check removed.
> 
OK
> chapters.{hpp,cpp}
> ------------------
> 
>   - introduced helper for `chapter_item_c::{Enter,Leave}` since they
>     are extremely similar in their implementation.

The whole Enter/Leave/EnterAndLeave part has been unused, untested, 
unmaintained and should have been broken since a loooooong time. If you 
want to clean the module, I strongly suggest that you remove it all :)

> 
>     The helper makes use of <algorithm> and <functional> to make the
>     code more error-proof, as a plus it is a little bit cleaner.
> ---
>  modules/demux/mkv/chapters.cpp | 70 
> +++++++++++++++++++-----------------------
>  modules/demux/mkv/chapters.hpp |  3 ++
>  2 files changed, 35 insertions(+), 38 deletions(-)
> 
> diff --git a/modules/demux/mkv/chapters.cpp 
> b/modules/demux/mkv/chapters.cpp
> index b8dab0c..6242143 100644
> --- a/modules/demux/mkv/chapters.cpp
> +++ b/modules/demux/mkv/chapters.cpp
> @@ -26,12 +26,13 @@
> 
>  #include "chapter_command.hpp"
> 
> +#include <functional>
> +#include <algorithm>
> +
>  chapter_item_c::~chapter_item_c()
>  {
> -    if( p_segment_uid )
> -        delete p_segment_uid;
> -    if( p_segment_edition_uid )
> -        delete p_segment_edition_uid;
> +    delete p_segment_uid;
> +    delete p_segment_edition_uid;
>      vlc_delete_all( codecs );
>      vlc_delete_all( sub_chapters );
>  }
> @@ -98,7 +99,7 @@ std::string chapter_item_c::GetCodecName( bool
> f_for_title ) const
>      while ( index != codecs.end() )
>      {
>          result = (*index)->GetCodecName( f_for_title );
> -        if ( result != "" )
> +        if ( !result.empty () )
>              break;
>          ++index;
>      }
> @@ -138,50 +139,43 @@ bool chapter_item_c::ParentOf( const
> chapter_item_c & item ) const
>      return false;
>  }
> 
> -bool chapter_item_c::Enter( bool b_do_subs )
> -{
> +bool chapter_item_c::EnterLeaveHelper_ ( bool do_subs,
> +    bool (chapter_codec_cmds_c::* co_cb) (),
> +    bool (chapter_item_c      ::* ch_cb) (bool)
> +) {
>      bool f_result = false;
> -    std::vector<chapter_codec_cmds_c*>::iterator index = 
> codecs.begin();
> -    while ( index != codecs.end() )
> -    {
> -        f_result |= (*index)->Enter();
> -        ++index;
> -    }
> 
> -    if ( b_do_subs )
> +    f_result |= std::count_if ( codecs.begin (), codecs.end (),
> +      std::mem_fun (co_cb)
> +    );
> +
> +    if ( do_subs )
>      {
> -        // sub chapters
> -        std::vector<chapter_item_c*>::iterator index_ = 
> sub_chapters.begin();
> -        while ( index_ != sub_chapters.end() )
> -        {
> -            f_result |= (*index_)->Enter( true );
> -            ++index_;
> -        }
> +        f_result |= count_if ( sub_chapters.begin (), 
> sub_chapters.end (),
> +          std::bind2nd( std::mem_fun( ch_cb ), true )
> +        );
>      }
> +
>      return f_result;
>  }
> 
> +bool chapter_item_c::Enter( bool b_do_subs )
> +{
> +    return EnterLeaveHelper_ (b_do_subs,
> +      &chapter_codec_cmds_c::Enter,
> +      &chapter_item_c::Enter
> +    );
> +}
> +
>  bool chapter_item_c::Leave( bool b_do_subs )
>  {
> -    bool f_result = false;
>      b_is_leaving = true;
> -    std::vector<chapter_codec_cmds_c*>::iterator index = 
> codecs.begin();
> -    while ( index != codecs.end() )
> -    {
> -        f_result |= (*index)->Leave();
> -        ++index;
> -    }
> 
> -    if ( b_do_subs )
> -    {
> -        // sub chapters
> -        std::vector<chapter_item_c*>::iterator index_ = 
> sub_chapters.begin();
> -        while ( index_ != sub_chapters.end() )
> -        {
> -            f_result |= (*index_)->Leave( true );
> -            ++index_;
> -        }
> -    }
> +    bool f_result = EnterLeaveHelper_ (b_do_subs,
> +      &chapter_codec_cmds_c::Leave,
> +      &chapter_item_c::Leave
> +    );
> +
>      b_is_leaving = false;
>      return f_result;
>  }
> diff --git a/modules/demux/mkv/chapters.hpp 
> b/modules/demux/mkv/chapters.hpp
> index 1b0885a..501c1cd 100644
> --- a/modules/demux/mkv/chapters.hpp
> +++ b/modules/demux/mkv/chapters.hpp
> @@ -88,6 +88,9 @@ public:
>      bool Enter( bool b_do_subchapters );
>      bool Leave( bool b_do_subchapters );
>      bool EnterAndLeave( chapter_item_c *p_item, bool b_enter = true 
> );
> +
> +  protected:
> +      bool EnterLeaveHelper_ (bool, bool(chapter_codec_cmds_c::*)(),
> bool(chapter_item_c::*)(bool));
>  };
> 
>  class chapter_edition_c : public chapter_item_c

Regards,
-- 
Denis Charmet - TypX
Le mauvais esprit est un art de vivre