Здравствуйте, DarkTranquillity, Вы писали:


H>>ну вот на i5 M540 @2.53 116 метровый файл из цифр обрабатывается за 2700 мс в 4 потока без ограничения по памяти, уверен, что можно быстрей.
DT>Хм.. Неплохо. Интересно, а если увеличить длину строки, и добавить в нее всяких экзотических не ASCII-символов?

длина строки в тестовом файле до 1000 символов, работает только с ASCII. На Unicode/UTF-8 не пробывал расширять.

Здравствуйте, DarkTranquillity, Вы писали:


H>>ну вот на i5 M540 @2.53 116 метровый файл из цифр обрабатывается за 2700 мс в 4 потока без ограничения по памяти, уверен, что можно быстрей.
DT>Хм.. Неплохо. Интересно, а если увеличить длину строки, и добавить в нее всяких экзотических не ASCII-символов?

Если использовать в сравнении строк memcmp(), то время работы не ухудшится, если использовать WideCharToMultibyte()/MultiByteToWideChar() + wcsncmp(), то время увеличится.

Здравствуйте, maykie, Вы писали:

M>

M>wmain()
M> if(!parse_cmd(conf,argc,argv)){
M> fprintf(stderr,usage);
M> return 0;
M> }

M>Не возвращай 0 из main() если произошла ошибка. За это бьют по рукам.
M>0 означает, что команда выполнена успешно. Я, кстати, не увидел, где бы пользователи сообщалось что что-то случилось. Код возврата fsort игнорируется. Ну и зачем он нужен?

консольная оболочка — только для тестов алгоритма во время разработки. каюсь, нужны были юнит-тесты.

M>

M>unsigned int __stdcall fsort_thread(void*param){
M> fsort_thread_arg_t*arg=(fsort_thread_arg_t*)param;
M> delete param;

M>за delete void* бить по рукам. Это undefined behaviour.

если бы передавался указатель на класс, то да, однозначно плохо, для С-структур — вполне.

M>

M>static unsigned int MAX_THREADS=8;

M>Мало. i7-980 поддерживает 12 потоков, к примеру.

пора апгрейды покупать.

M>

M>private:
M> // deny
M> buffer_t(buffer_t const&)throw();
M> buffer_t&operator=(buffer_t const&)throw();

M>зачем говорить что они throw()?

я за то, что бы конструктор, конструктор копирования, деструктор и оператор присваивания не генерировали исключений. Но в связи с последними комментариями, такое не всегда можно сделать, особенно, если политика обработки ошибок базируется на исключениях. Вроде Саттер писал что-то по этому поводу в одной из "Эффективное использование C++".

M>Да, кстати, писать const справа это достаточно редкий стиль.

Так понятней к чему const относится.

Спасибо за ревью!

Здравствуйте, Hydrophobia, Вы писали:

H>Здравствуйте, DarkTranquillity, Вы писали:


H>>>ну вот на i5 M540 @2.53 116 метровый файл из цифр обрабатывается за 2700 мс в 4 потока без ограничения по памяти, уверен, что можно быстрей.
DT>>Хм.. Неплохо. Интересно, а если увеличить длину строки, и добавить в нее всяких экзотических не ASCII-символов?

H>Если использовать в сравнении строк memcmp(), то время работы не ухудшится, если использовать WideCharToMultibyte()/MultiByteToWideChar() + wcsncmp(), то время увеличится.

Вы просто капитан Очевидность!
Использовать простое копирование памяти, или вызовы трех не столь нетривиальных функций.
Вопрос в другом — при сравнении строк ведь есть еще такое понятие как collate.

Здравствуйте, DarkTranquillity, Вы писали:

DT>Вопрос в другом — при сравнении строк ведь есть еще такое понятие как collate.

в смысле лексикографический порядок?

Здравствуйте, Hydrophobia, Вы писали:

H>Здравствуйте, DarkTranquillity, Вы писали:

DT>>Вопрос в другом — при сравнении строк ведь есть еще такое понятие как collate.

H>в смысле лексикографический порядок?

Да.

Здравствуйте, DarkTranquillity, Вы писали:

DT>>>Вопрос в другом — при сравнении строк ведь есть еще такое понятие как collate.

H>>в смысле лексикографический порядок?

DT>Да.

по хорошему, он должен быть указан в задаче. Тот же sort из coreutils имеет параметры, задающие те или иные правила сравнения (--general-numeric-sort, --ignore-nonprinting, --ignore-case и т.д.).

Здравствуйте, Hydrophobia, Вы писали:

Посмотрел fsort с точки зрения чистоты кода. Если код писался на время — то это не существенно, если на качество — существенно.

Функция fsort очень длинная. По хорошему, функции должны быть на пол экрана, не больше. Каждая функция должна выражать одну идею и быть однородной по составу. Название функции должно выражать то что она делает, так чтобы не нужно было заглядывать внутрь, чтобы вскрывать нюансы.
Больше автоматики, HANDLE и голые указатели это прошлый век.

H>От Вас необходимы замечания и конструктивная критика кода.
>unsigned int fsort(std::wstring const&in,std::wstring const&out,fsort_conf_t const&conf){
тытекстытожепишешьбезпробелов?

>buffer_t*buffer=new buffer_t(in,conf.max_size_);
потенциальный меморилик + необходимость тратить ценное внимание на контроль

>std::vector<HANDLE>
в общем случае лучше использовать автоматику

на мой взгляд именования классов с _t стоит использовать когда косишь под стандарт (но это субъективно)

>

>  return result<0?true
>                 :(result==0)?str0.length_<str1.length_
>                             :false;
>

Я бы написал if, такие вещи нужно использовать только в очень простых случаях.

Смысл использования FSORT_NAMESPACE_BEGIN не очевиден и в примере ИМХО это лишнее.

>typedef struct{
>} merge2files_thread_arg_t;
Смысл писать такие вещи в 21 веке?

>#include <windows.h>
Лучше не использовать в хидере. Как же потенциальная кроссплатформенность?

>unsigned int __stdcall fsort_thread(void*param);
>unsigned int __stdcall merge2files_thread(void*param);
смысл этого объявления?

Здравствуйте, Hydrophobia, Вы писали:

M>>за delete void* бить по рукам. Это undefined behaviour.

H>если бы передавался указатель на класс, то да, однозначно плохо, для С-структур — вполне.

Нет. Помимо вызова деструктора delete может использовать информацию о размере объекта.

M>>Да, кстати, писать const справа это достаточно редкий стиль.

H>Так понятней к чему const относится.

Мне _const нравится больше, но обычно он не используется, к сожалению.

Здравствуйте, Hydrophobia, Вы писали:

1. goto? Вы не на ассемблере пишите, чтобы таким пользоваться, я бы уже только за это ваш проект в мусорку выкинул.
2. Где комментарии в функциях. Есть комментарии к функциям, но нет комментариев в телах функций.
3. Не понял зачем namespace объявлены.

Здравствуйте, Glas, Вы писали:

G>2. Где комментарии в функциях. Есть комментарии к функциям,
Кстати — зря, лучше бы более читаемое название функций было сделано.

G>но нет комментариев в телах функций.
Они вредны.

A>Это С-style код с некоторыми фичами С++.
A>Код на С есть смысл писать только там где нельзя использовать С++ (некоторые микроконтроллеры, драйвера), сортировка строк в файле под виндой — это очевидно не та ситуация.
A>Как С++ код, это полное гуано. На С++ так не пишут — надо использовать умные указатели, RAII, никакого delete, и уж точно никакого goto cleanup


знаешь, интервьюверы нередко (именно в майкрософт и еще нескольких крупных конторах) просят на собеседовании
писать код на простом C, чтобы понять суть алгоритма и не отвлекаться на непонятные синтаксические конструкции

Здравствуйте, Nikе, Вы писали:

G>>но нет комментариев в телах функций.
N>Они вредны.

Они могут быть вредны в очевидных местах и в непосредственной разработке. В данном случае заказчику надо быстро и внятно показать, что делает код или пусть как хочет разбирается?

Здравствуйте, Glas, Вы писали:

G>>>но нет комментариев в телах функций.
N>>Они вредны.

G>Они могут быть вредны в очевидных местах и в непосредственной разработке. В данном случае заказчику надо быстро и внятно показать, что делает код или пусть как хочет разбирается?

Я исхожу из правила, что необходимость комментария в коде говорит нам о необходимости рефакторинга.
Можно написать комментарий. При этом код будет продублирован и по сути станет сложнее, так как появится необходимость поддерживать не только плохой код, но и комментарий.
Хороший рефакторинг — упростит ситуацию и сделает суппорт дешевле.

Здравствуйте, Nikе, Вы писали:

N>Можно написать комментарий. При этом код будет продублирован и по сути станет сложнее, так как появится необходимость поддерживать не только плохой код, но и комментарий.
Как это комментарий дублирует код?
ЗЫ всегда думал, что комментарии нужны, чтобы другие программисты быстрее понимали, как это работает.

Я где-то уже это слышал, очень много кода...

Логичный отлуп получил.

Здравствуйте, Glas, Вы писали:

N>>Можно написать комментарий. При этом код будет продублирован и по сути станет сложнее, так как появится необходимость поддерживать не только плохой код, но и комментарий.
G>Как это комментарий дублирует код?
Код описывает функциональность, комментарий описывает функциональность. Дубль.
Код должен понятно описывать функциональность. Конкретно С++ даёт множество инструментов для этого. Если код это не делает — то это плохой код и его нужно делать хорошим, а не добавлять костыли.

G>ЗЫ всегда думал, что комментарии нужны, чтобы другие программисты быстрее понимали, как это работает.
Для ускорения понимания нужно добавлять примеры и высокоуровневое описание структуры. А для того чтобы понять как работает код — надо читать код, никак иначе.

Здравствуйте, Glas, Вы писали:

G>Как это комментарий дублирует код?

Два банальных примера плохого использования комментариев:
Комментарий в объявлении:

// merge 2 blocks into one
void merge2blocks(std::vector<line_t> const&block0,std::vector<line_t> const&block1,std::vector<line_t>&result);

Дубль + возможно не очень хорошее именование.

Комментарий в cpp:

  // merge temporary files

Ясный детектор необходимости новой функции: MergeFiles( temporary ).

Здравствуйте, Hydrophobia, Вы писали:

Ещё:
рекомендую использовать: typedef std::vector<Line> Lines; а не совать повсюду std::vector<line_t>, так как:
1. короче и понятнее (дано имя).
2. если захочется поменять контейнер, добавить аллокатор, прорефакторить — будет проще это сделать.

Код даже комментить не хочу, по нему можно трактат написать как не надо программировать.

Вообще-то Вы реализовали unix sort. Надо бы объяснить в комментах какой алгоритм использован и почему. Код неимоверно сложен. Парсинг командной строки ручками — это такое извращение? Или stdarg.h уже в винде отменили? Я например не понял, зачем нужна многопоточность и как она соотносится с блокирующими операциями чтения диска.

Все жутко перемешано вместе. Алгоритмика должна быть отдельно, ввод/вывод отдельно. Очень плохо структурированы модули и невнятные интерфейсы между ними. STL используется не по делу. Мешанина из типов Win32/STL/C

Надо сделать куда проще и понятней.