rikyoz / bit7z

A C++ static library offering a clean and simple interface to the 7-zip shared libraries.
https://rikyoz.github.io/bit7z
Mozilla Public License 2.0
623 stars 113 forks source link

std::sort fails on BitArchiveInfo::items #54

Closed fire-eggs closed 2 years ago

fire-eggs commented 2 years ago

Bit7z V3.1.3 Visual Studio 2019

Thought I'd sort the vector by filename:

bool sortfunc(BitArchiveItem a, BitArchiveItem b) { return a.name() < b.name(); }
...
BitArchiveInfo bai{ lib, path, BitFormat::Rar };
auto arc_items = bai.items();
std::sort(arc_items.begin(), arc_items.end(), sortfunc);

but got three of these utterly confusing compile errors:

1>E:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\algorithm(7247,25): error C2280: 'bit7z::BitArchiveItem &bit7z::BitArchiveItem::operator =(const bit7z::BitArchiveItem &)': attempting to reference a deleted function
1>E:\proj\DarkThumbs2\DT_Bit7z\bit7z\include\bitarchiveitem.hpp(106): message : compiler has generated 'bit7z::BitArchiveItem::operator =' here
1>E:\proj\DarkThumbs2\DT_Bit7z\bit7z\include\bitarchiveitem.hpp(106,5): message : 'bit7z::BitArchiveItem &bit7z::BitArchiveItem::operator =(const bit7z::BitArchiveItem &)': function was implicitly deleted because 'bit7z::BitArchiveItem' has a data member 'bit7z::BitArchiveItem::mItemIndex' of const-qualified non-class type
1>E:\proj\DarkThumbs2\DT_Bit7z\bit7z\include\bitarchiveitem.hpp(97): message : see declaration of 'bit7z::BitArchiveItem::mItemIndex'
1>E:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\algorithm(7369): message : see reference to function template instantiation '_BidIt *std::_Insertion_sort_unchecked<_RanIt,bool(__cdecl *)(bit7z::BitArchiveItem,bit7z::BitArchiveItem)>(const _BidIt,const _BidIt,_Pr)' being compiled
1>        with
1>        [
1>            _BidIt=bit7z::BitArchiveItem *,
1>            _RanIt=bit7z::BitArchiveItem *,
1>            _Pr=bool (__cdecl *)(bit7z::BitArchiveItem,bit7z::BitArchiveItem)
1>        ]
1>E:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\algorithm(7399): message : see reference to function template instantiation 'void std::_Sort_unchecked<bit7z::BitArchiveItem*,bool(__cdecl *)(bit7z::BitArchiveItem,bit7z::BitArchiveItem)>(_RanIt,_RanIt,__int64,_Pr)' being compiled
1>        with
1>        [
1>            _RanIt=bit7z::BitArchiveItem *,
1>            _Pr=bool (__cdecl *)(bit7z::BitArchiveItem,bit7z::BitArchiveItem)
1>        ]
1>E:\proj\DarkThumbs2\DT_Bit7z\rar.cpp(31): message : see reference to function template instantiation 'void std::sort<std::_Vector_iterator<std::_Vector_val<std::_Simple_types<_Ty>>>,bool(__cdecl *)(bit7z::BitArchiveItem,bit7z::BitArchiveItem)>(const _RanIt,const _RanIt,_Pr)' being compiled
1>        with
1>        [
1>            _Ty=bit7z::BitArchiveItem,
1>            _RanIt=std::_Vector_iterator<std::_Vector_val<std::_Simple_types<bit7z::BitArchiveItem>>>,
1>            _Pr=bool (__cdecl *)(bit7z::BitArchiveItem,bit7z::BitArchiveItem)
1>        ]

I get the same result even if I create a copy of the items() vector.

Any hints? Thanks!

rikyoz commented 2 years ago

Hi! Did you try to use a sortfunc taking constant reference arguments? Something like bool sortfunc(const BitArchiveItem& a, const BitArchiveItem& b) { return a.name() < b.name(); }. It should also optimize the operation since you avoid unnecessary copies of the BitArchiveItem objects you compare.

The main reason for the error you're getting is that the BitArchiveItem class has a const member variable (i.e., the item index inside the archive, which doesn't change, hence the const-ness). This variable disables the generation of an implicit copy assignment operator, and the class doesn't specify a custom one. Hence you cannot pass by value a BitArchiveItem object to a function like in your sortfunc, since it requires the ability to copy the objects. Passing by reference should fix the problem.

As a side note, copying BitArchiveItems could be expensive since they contain a map of every property it has. Most of the time is not the desired operation!

fire-eggs commented 2 years ago

Much obliged for the quick response! Alas, I still get the same complaints, with the following sortfunc:

bool sortfunc(const BitArchiveItem& a, const BitArchiveItem& b) { return a.name() < b.name(); }

[C++ templates make my head hurt ... ]

fire-eggs commented 2 years ago

The portion of the messages which references algorithm (e.g.

1>E:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\algorithm(7247,25): error C2280: 'bit7z::BitArchiveItem &bit7z::BitArchiveItem::operator =(const bit7z::BitArchiveItem &)': attempting to reference a deleted function

is where _Insertion_sort_unchecked is trying to move vector elements around [I've marked the three lines with HERE]:

template <class _BidIt, class _Pr>
_CONSTEXPR20 _BidIt _Insertion_sort_unchecked(const _BidIt _First, const _BidIt _Last, _Pr _Pred) {
    // insertion sort [_First, _Last)
    if (_First != _Last) {
        for (_BidIt _Mid = _First; ++_Mid != _Last;) { // order next element
            _BidIt _Hole               = _Mid;
            _Iter_value_t<_BidIt> _Val = _STD move(*_Mid);

            if (_DEBUG_LT_PRED(_Pred, _Val, *_First)) { // found new earliest element, move to front
                _Move_backward_unchecked(_First, _Mid, ++_Hole);
HERE                *_First = _STD move(_Val);
            } else { // look for insertion point after first
                for (_BidIt _Prev = _Hole; _DEBUG_LT_PRED(_Pred, _Val, *--_Prev); _Hole = _Prev) {
HERE                    *_Hole = _STD move(*_Prev); // move hole down
                }

HERE                *_Hole = _STD move(_Val); // insert element in hole
            }
        }
    }
fire-eggs commented 2 years ago

and I should say it's not a show-stopper, just an unexpected result.

rikyoz commented 2 years ago

Yeah, I didn't consider that std::sort requires the type stored in the container to have a move constructor and a move assignment operator. And also both those implicit functions are deleted by default due to the constant member variable, hence the error. I'm beginning to think that defining that member variable as const was probably a poor design decision. I'll evaluate whether to change it. In that case, I'll have to release a fixed build. Thank you very much!

rikyoz commented 2 years ago

Fixed in v3.1.4

fire-eggs commented 2 years ago

Much obliged for the update!