Re: [boost] [review] Last day for formal review for Sort library

Subject: Re: [boost] [review] Last day for formal review for Sort library
From: DUPUIS Etienne (e.dupuis_at_[hidden])
Date: 2014-11-20 10:32:35

Greetings,

I do not intend to send a formal review but I nevertheless compiled and
executed the provided test code, with clang 3.5. I added timings and
confirmed that in the provided test case, this sort is significantly
faster than std::sort() provided by the latest Linux Mint distribution.

To the author, I would suggest two very minor issues :

1. There are compiler warnings that can easily be avoided (seen with
'-Wunused-parameter' and '-Wold-style-cast').
2. In string_sort_test.cpp, tests are performed using an array of 32
elements. 10 000 or more should make a more convincing test.

I have also seen in the documentation that samples are available.
Indeed, they are in git. However, I would appreciate samples  in the
documentation, without any useless I/O code (useless to understand how
to use the sort functions).

For example, taken from 'keyplusdatasample.cpp' :

// --------------- Start of code snippet ------------------------

struct DATA_TYPE {
     int key;
     std::string data;
};
struct lessthan {
     inline bool operator()(const DATA_TYPE &x, const DATA_TYPE &y) const
{
         return x.key < y.key;
     }
};

struct rightshift {
     inline int operator()(const DATA_TYPE &x, const unsigned offset) {
         return x.key >> offset;
     }
};

std::vector<DATA_TYPE> array;
integer_sort(array.begin(), array.end(), rightshift(), lessthan());

// --------------- End of code snippet ------------------------
Having concise code snippets like the above in the documentation is in
my opinion the best way to have users quickly understand how the library
must be used.

I would also like to see one or two more example using "complex" data
types. Consider for example :

// --------------- Start of code snippet ------------------------
struct Md5Sum {
     uint8_t bytes[16];

     bool operator<(const Md5Sum& md5) const {
         for (int k = 0; k < 16; k++)
             if (bytes[k] < md5.bytes[k])
                 return true;
        return false;
     }
};

// --------------- End of code snippet ------------------------
After reading your documentation and the provided samples, I understand
that I should use string_sort() after defining some get_char() and
get_length() functions. Am I right ?

Another example, sorting by birth data and then by name :

// --------------- Start of code snippet ------------------------
struct Id {
     std::string name;
     time_t birth;

     bool operator<(const Id& id) const {
         return (birth < id.birth) || ((birth == id.birth) && (name <
id.name));
     }
};

// --------------- End of code snippet ------------------------
Again, I understand that I must use string_sort and split the birth date
into 8-bit components. Am I right ?

These samples are important as there is probably much more people
sorting structures than simple integers.



Regards,
Étienne




------ Message d'origine ------
De : "Edward Diener" <eldiener_at_[hidden]>
À : "boost_at_[hidden]" <boost_at_[hidden]>
Cc: "boost-users_at_[hidden]" <boost-users_at_[hidden]>
Envoyé 19/11/2014 13:30:53
Objet : [boost] [review] Last day for formal review for Sort library

>Today is the last day for the formal review of the Sort library by
>Steven Ross, which started November 10.
>
>We have had some great discussions and reviews so far and I would like
>to thank everybody involved, but as always we need more formal reviews.
>If you need any delay for your review after today, please contact me at
>'eldiener at tropicsoft dot com' and I will take your review into
>account if given within a reasonable period of time after today.
>
>
This message and any attachments are confidential and intended solely for the addressees. Any unauthorized modification, edition, use or dissemination is prohibited. If you have received this message by mistake, please notify us immediately. ATEME decline all responsibility for this message if it has been altered, deformed, falsified or even edited or disseminated without authorization.