Awaiting Review
От: malegkin  
Дата: 12.02.19 10:40
Оценка:
Добрый день.
Давненько я не брал в руки плюсы...
Буду признателен за критику кода решения задачки http://acm.timus.ru/problem.aspx?space=1&num=1712
Волнует в первую очередь move semantics / передача и возврат объектов между функциями.
Возможно ли repeat сделать проще? ну лень мне каждый раз писать for(;) ))

#include <iostream>
#include <string>
#include <array>
#include <algorithm>
#include <utility>
#include <iterator>

static const size_t SQUARE_DIMENSION = 4;
using square_t = std::array<std::string, SQUARE_DIMENSION>;

template <typename F>
void repeat(unsigned n, F f) {
    while (n--) f();
}

inline square_t read_square(std::istream &in){
    square_t out;
    
    std::generate_n( std::begin(out), SQUARE_DIMENSION, [&](){
        std::string line;
        getline(in, line);
        return std::move( line ); 
    });

    return std::move( out );
}

inline square_t& rotate(square_t &in){

    for (size_t i = 0; i < SQUARE_DIMENSION; i++) {
        for (size_t j = i; j < SQUARE_DIMENSION; j++) {
            std::swap( in[i][j], in[j][i] ); 
        }
    }


    for(size_t i = 0; i < SQUARE_DIMENSION/2; i++) {
        for(size_t j = 0; j < SQUARE_DIMENSION; j++) {
            std::swap( in[j][i], in[j][SQUARE_DIMENSION - i - 1] );
        }
    }
    
    return in;
}


inline std::string merge(const square_t &ciphered, const square_t &grille){
    std::string out;

    for (size_t i = 0; i < SQUARE_DIMENSION; i++){
        for (size_t j = 0; j < SQUARE_DIMENSION; j++){
            if ( grille[i][j] == 'X' )
                out += ciphered[i][j];
        }
    }
    

    return std::move( out );
}


int main()
{
    auto grille     = read_square( std::cin );
    auto ciphered   = read_square( std::cin );


    repeat(4, [&](){
        std::cout << merge( ciphered, grille );
        rotate(grille);
    });

    return 0;
}
Re: Awaiting Review
От: watchmaker  
Дата: 12.02.19 12:08
Оценка: 3 (1)
Здравствуйте, malegkin, Вы писали:

M>Волнует в первую очередь move semantics / передача и возврат объектов между функциями.

С этим тут плохо.
Простой return out; и короче записывается и работать будет быстрее.
А return std::move( out ); — это пессимизация.

Код вида return std::move(something) нужен совсем в других ситуациях (очень редких), например во всяких string_builder'ах:
S&& append(S&& obj, const T& diff) {
  obj.apply(diff);
  return std::move(obj);
}
Отредактировано 12.02.2019 12:09 watchmaker . Предыдущая версия .
Re: Awaiting Review
От: sergii.p  
Дата: 12.02.19 12:49
Оценка: 3 (1)
Здравствуйте, malegkin, Вы писали:

1. Конструкции return std::move обычно вредны, потому как ломают RVO.
2. в функции rotate() цикл видимо должен начинаться так for (size_t j = i + 1 ... иначе не понятно кто с кем меняется. Вообще функция мутная, я что-то не разобрался, что она делает
3. в функции merge неплохо бы сделать out.reserve(SQUARE_DIMENSION). Также это неплохо бы сделать в функции read_square() line.reserve(SQUARE_DIMENSION) потому как функция getline вычитывает из потока посимвольно

в остальном мне код понравился
Re: Awaiting Review
От: rg45 СССР  
Дата: 12.02.19 13:24
Оценка: 3 (1) +1
Здравствуйте, malegkin, Вы писали:

M>
M>template <typename F>
M>void repeat(unsigned n, F f) {
M>    while (n--) f();
M>}
M>


В качестве наиболее универсального способа передачи функтора в функцию имеет смысл использовать форвардную ссылку:

template <typename F>
void repeat (usnsigned n, F&& f) {
    while (n--) f();
}


Это не обычная rvalue ссылка, как может показаться на первый взгляд, а так называемая forwarding reference. Она может превратиться как в rvalue ссылку, так и в lvalue — константную, или неконстантную. Тем самым обеспечит автовыведение наиболее подходящего типа ссылки, подобранного в соответствии с типом фактического параметра, и убережет от нежелательного копирования/перемещения, что может быть актуально для "тяжелых" функциональных объектов и лямбд.
--
Не можешь достичь желаемого — пожелай достигнутого.
Отредактировано 12.02.2019 13:32 rg45 . Предыдущая версия .
Re: Awaiting Review
От: qaz77  
Дата: 12.02.19 14:22
Оценка: 3 (1)
Здравствуйте, malegkin, Вы писали:

M>static const size_t SQUARE_DIMENSION = 4;

Может мелкая придирка, но я в своем коде стараюсь не использовать all upper case имена,
т.к. есть вероятность пересечься с каким-нибудь макросом.

M> return std::move( out );

В return std::move не нужен. Компилятор сам сделает перемещение при возврате по значению и доступности перемещения.
Для std::string и массива из них перемещение, конечно, полезно.

M>inline square_t& rotate(square_t &in){

M> ...
M> return in;
M>}
Странная сигнатура функции. Принимаем lvalue и его же возвращаем?
Какие-то цепочки преобразований записывать?
Для меня было бы естественней: void rotate(square_t &in)
Re[2]: Awaiting Review
От: malegkin  
Дата: 12.02.19 22:22
Оценка:
Здравствуйте, watchmaker, Вы писали:

M>>Волнует в первую очередь move semantics / передача и возврат объектов между функциями.

W>С этим тут плохо.
W>Простой return out; и короче записывается и работать будет быстрее.

спасибо за пояснение. давно читал про RVO, казалось, что он требует явного std::move (
Re[2]: Awaiting Review
От: malegkin  
Дата: 12.02.19 22:31
Оценка:
Здравствуйте, sergii.p, Вы писали:

SP>1. Конструкции return std::move обычно вредны, потому как ломают RVO.

ээх плохо и давно про RVO читал, казалось наоборот ( пошел перечитывать )

SP>2. в функции rotate() цикл видимо должен начинаться так for (size_t j = i + 1 ... иначе не понятно кто с кем меняется. Вообще функция мутная, я что-то не разобрался, что она делает

ага мутная ) тестов для нее еще не хватает , но в задачке проще было отладить через принт-дебаг ) а так она норм работает. поворачивает решетку Кардано для одноименного шифра ) в


SP>3. в функции merge неплохо бы сделать out.reserve(SQUARE_DIMENSION). Также это неплохо бы сделать в функции read_square() line.reserve(SQUARE_DIMENSION) потому как функция getline вычитывает из потока посимвольно


в общем случае для произвольного SQUARE_DIMENSION само собой. спасибо за напоминание, не подумал об этом. но для 4 и соответсвенно 16 символов в блоке, Small String Optimisation должны меня спасти ) там какраз, кажется 16 символов )

SP>в остальном мне код понравился


спасибо
Re[2]: Awaiting Review
От: malegkin  
Дата: 12.02.19 22:35
Оценка:
Здравствуйте, rg45, Вы писали:

R>Это не обычная rvalue ссылка, как может показаться на первый взгляд, а так называемая forwarding reference. Она может превратиться как в rvalue ссылку, так и в lvalue — константную, или неконстантную. Тем самым обеспечит автовыведение наиболее подходящего типа ссылки, подобранного в соответствии с типом фактического параметра, и убережет от нежелательного копирования/перемещения, что может быть актуально для "тяжелых" функциональных объектов и лямбд.


Спасибо большое
Ох намудрили же в плюсах с функторами .. rvalue/lvalue это отдельная боль для меня. пошел курить доки )
Re[2]: Awaiting Review
От: malegkin  
Дата: 12.02.19 22:51
Оценка:
Здравствуйте, qaz77, Вы писали:

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


M>>static const size_t SQUARE_DIMENSION = 4;

Q>Может мелкая придирка, но я в своем коде стараюсь не использовать all upper case имена,
Q>т.к. есть вероятность пересечься с каким-нибудь макросом.

спасибо за замечание. вы правы. но ради маленькой задачки очень лень местами заморачиваться )

M>> return std::move( out );

Q>В return std::move не нужен. Компилятор сам сделает перемещение при возврате по значению и доступности перемещения.
Q>Для std::string и массива из них перемещение, конечно, полезно.

Да уж возврат из функции новых объектов , особенно с учетом того, что RVO как я понял может и не отработать, это печаль боль ((

M>>inline square_t& rotate(square_t &in){

M>> ...
M>> return in;
M>>}
Q>Странная сигнатура функции. Принимаем lvalue и его же возвращаем?
Q>Какие-то цепочки преобразований записывать?
Q>Для меня было бы естественней: void rotate(square_t &in)

спасибо, что-то не обращал внимания, что алгоритмы стандартной библиотеки, также не возвращают ничего из себя. как-то со времен перегрузки операторов разных осталась привычка возвращать объект назад ) что в данном случае излишне. использование rotate(rotate(rotate(it))) это ахтунг какой-то )
Re: Awaiting Review
От: PM  
Дата: 13.02.19 06:41
Оценка: 3 (1)
Здравствуйте, malegkin, Вы писали:

M>Давненько я не брал в руки плюсы...

M>Буду признателен за критику кода решения задачки http://acm.timus.ru/problem.aspx?space=1&amp;num=1712
M>Волнует в первую очередь move semantics / передача и возврат объектов между функциями.
M>Возможно ли repeat сделать проще? ну лень мне каждый раз писать for(;) ))

Мои 5 копеек — inline не нужен практически всегда. Единственное его применение — определение функции в заголовочных файлах, чтобы предотвратить ошибки линкора о множественных именах для такой функции (а также inline variable начиная с C++17).

Еще вместо std::string для строк square_t можно сделать двумерный массив символов, но это вопрос вкуса.

И repeat можно сделать проще, написав обычный цикл for, знакомый 100% читателей
Re: Awaiting Review
От: kov_serg Россия  
Дата: 13.02.19 07:57
Оценка: -1
Здравствуйте, malegkin, Вы писали:

M>Добрый день.

M>Давненько я не брал в руки плюсы...
M>Буду признателен за критику кода решения задачки http://acm.timus.ru/problem.aspx?space=1&amp;num=1712
M>Волнует в первую очередь move semantics / передача и возврат объектов между функциями.
M>Возможно ли repeat сделать проще? ну лень мне каждый раз писать for(;) ))
Можно вообще без плюсов сделать
#include <stdio.h>
#include <stdlib.h>

enum { N=4, NN=N*N };
int rot(int c) { return N*(c%N)+N-1-c/N; }
int cmp(const void* a, const void* b){ return *(int*)a - *(int*)b; }
void sort(int *x,int n) { qsort(x,n,sizeof(*x),cmp); }
void read(char* s) {
    int x,y,i,j; enum { line_max=80 }; char line[line_max];
    for(i=0,y=0;y<N;y++) {
        fgets(line,line_max,stdin);
        for(j=0,x=0;x<N;x++) {
            s[i++]=line[j];
            if (line[j]) j++;
        }
    }    
}
int main(int argc,char** argv) {
    int i,j, nxy, xy[NN]; char s[NN];
    read(s); 
    nxy=0; for(i=0;i<NN;i++) if (s[i]=='X') xy[nxy++]=i;
    read(s);     
    for(i=0;i<4;i++) {
        for(j=0;j<nxy;j++) { 
            putc(s[xy[j]],stdout);
            xy[j]=rot(xy[j]);
        }
        sort(xy,nxy);
    }
    putc('\n',stdout);
    return 0;
}
Re[2]: Awaiting Review
От: Skorodum Россия  
Дата: 13.02.19 09:23
Оценка:
Здравствуйте, kov_serg, Вы писали:

"-" за форматирование
Re[2]: Awaiting Review
От: malegkin  
Дата: 13.02.19 09:59
Оценка:
Здравствуйте, PM, Вы писали:

PM>Мои 5 копеек — inline не нужен практически всегда. Единственное его применение — определение функции в заголовочных файлах, чтобы предотвратить ошибки линкора о множественных именах для такой функции (а также inline variable начиная с C++17).


о про inline variable не знал, спасибо. Хотя у меня C++14 пока в проработке )

PM>И repeat можно сделать проще, написав обычный цикл for, знакомый 100% читателей

вся прелесть рипита, на мой взгляд, какраз в том, чтобы не писать постоянно for(;){} для банального повторение 10 раз какого-то тупого действия. Понятно, что в боевом коде, это избыточно, но во всяких задачках, постоянно встречается необходимость повторить N раз какое-то тупое действие, причем по нескольку раз за задачу ) имхо, жалко, что сам язык не позволяет делать фишки типа:
5*[]{ 
    std::cout << "HELLO WORLD";
}
Re[3]: Awaiting Review
От: kov_serg Россия  
Дата: 13.02.19 11:19
Оценка:
Здравствуйте, Skorodum, Вы писали:

S>"-" за форматирование

http://format.krzaq.cc/



Как мне нравиться когда к форме повествование требование выше чем к содержанию.
Напоминает современный кинематограф когда спецэффекты важнее сюжета.
Отредактировано 14.02.2019 6:02 kov_serg . Предыдущая версия . Еще …
Отредактировано 13.02.2019 18:04 kov_serg . Предыдущая версия .
Re: Awaiting Review
От: EreTIk EreTIk's Box
Дата: 13.02.19 12:38
Оценка: 1 (1) +1
Здравствуйте, malegkin, Вы писали:

Мои 5 копеек:

M>
M>static const size_t SQUARE_DIMENSION = 4;
M>


В современных плюсах предпочтительнее использовать constexpr для констант и функций, вычислимых на этапе компиляции.
Re[2]: Awaiting Review
От: Skorodum Россия  
Дата: 13.02.19 12:39
Оценка:
Здравствуйте, sergii.p, Вы писали:

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


SP>1. Конструкции return std::move обычно вредны, потому как ломают RVO.

А на RVO есть хоть какая-то гарантия? Вроде же там как левая пятка компилятора захочет
Если же нужна гарантия, то ничего лучше указателей вроде еще не придумали. Или нет?
Re[3]: Awaiting Review
От: watchmaker  
Дата: 13.02.19 12:55
Оценка: 8 (3)
Здравствуйте, Skorodum, Вы писали:


S>А на RVO есть хоть какая-то гарантия? Вроде же там как левая пятка компилятора захочет

S>Если же нужна гарантия, то ничего лучше указателей вроде еще не придумали. Или нет?

Guaranteed copy elision — в C++17 компилятор не имеет право копировать/перемещать объект. Даже если полностью выключить оптимизацию, и даже если его попробовать явно об этом попросить. Например, в gcc или clang можно передать ключ -fno-elide-constructors и сравнить генерируемый код для C++14 и C++17 — во втором случае return Object{...}; не будет копироваться или перемещаться.

(это именно для RVO, для NVRO чуть другие правила, да).

Upd:
На всякий случай уточню, это guaranteed copy elision введено не для оптимизации (это побочный эффект), а для того, чтобы можно было на С++ фабрики нормально писать и возвращать из функций экземпляры классов, у которых нет конструкторов копирования или перемещения:
class Obj {
private:
    Obj(int); 
public:
    Obj(Obj&&) = delete;
    Obj(const Obj&) = delete;
};

Obj CreateObj() { 
  return Obj(4);  // нельзя до C++17
}
Отредактировано 13.02.2019 13:24 watchmaker . Предыдущая версия .
Re[2]: Awaiting Review
От: _NN_ www.nemerleweb.com
Дата: 14.02.19 13:44
Оценка:
Здравствуйте, qaz77, Вы писали:

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


M>>static const size_t SQUARE_DIMENSION = 4;

Q>Может мелкая придирка, но я в своем коде стараюсь не использовать all upper case имена,
Q>т.к. есть вероятность пересечься с каким-нибудь макросом.

Всю жизнь использовал и не было проблем с пересечением.
Вменяемые писатели макросов всегда используют какой-нибудь уникальный префикс как BOOST_PP_*.
http://rsdn.nemerleweb.com
http://nemerleweb.com
Re[3]: Awaiting Review
От: qaz77  
Дата: 15.02.19 08:22
Оценка: 1 (1) +2
Здравствуйте, _NN_, Вы писали:

_NN>Всю жизнь использовал и не было проблем с пересечением.

А вот в моей практике были, правда давно, когда студия макросы в коде еще не умела подсвечивать.

_NN>Вменяемые писатели макросов всегда используют какой-нибудь уникальный префикс как BOOST_PP_*.

Вменяемые — да. Как позитивный пример могу привести CURL.
Проблемы начинаются после #include <windows.h>, где куча макросов без всяких префиксов.
Но, справедливости ради, там также куча макросов с буквами в разных регистрах (всякие A/W функции, например) и
min/max — классика жанра.
Re[4]: Awaiting Review
От: _NN_ www.nemerleweb.com
Дата: 15.02.19 21:34
Оценка:
Здравствуйте, qaz77, Вы писали:

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


_NN>>Всю жизнь использовал и не было проблем с пересечением.

Q>А вот в моей практике были, правда давно, когда студия макросы в коде еще не умела подсвечивать.

_NN>>Вменяемые писатели макросов всегда используют какой-нибудь уникальный префикс как BOOST_PP_*.

Q>Вменяемые — да. Как позитивный пример могу привести CURL.
Q>Проблемы начинаются после #include <windows.h>, где куча макросов без всяких префиксов.
Q>Но, справедливости ради, там также куча макросов с буквами в разных регистрах (всякие A/W функции, например) и
Q>min/max — классика жанра.

Работать с windows.h без NOMINMAX ещё 20 лет назад было совсем не модно
А так да, там много чего определенно, но не припомню когда в последний раз были коллизии.

Ещё полезно добавлять весь набор NO**: NOGDICAPMASKS, NOVIRTUALKEYCODES, ... и WIN32_LEAN_AND_MEAN.
Так список возможных пересечений уменьшиться да и собираться быстрее будет.

https://github.com/tpn/winsdk-10/blob/master/Include/10.0.10240.0/um/Windows.h#L38
http://rsdn.nemerleweb.com
http://nemerleweb.com
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.