Создаю библиотеку для работы с последовательными портами. [УШЕЛ ИЗ ПРОЕКТА].

[b-s-a]

Ревизия текущего состояния библиотеки:
1. Считаю, что методы SerialPortPrivate::setReadNotificationEnabled и пр. следует перенести в AbstractSerialPortPrivate. В том числе и нотификатор, дабы не плодить сущностей.
2. Считаю, что SerialPort не должен вызывать приватные/защищенные методы SerialPortPrivate/AbstractSerialPortPrivate (начал работать с этим, но лучше пусть автор сделает)
3. serialport_p.h, думаю, стоит разделить по архитектурам. Не нравятся мне такие нагромождения.

Работоспособность библиотеки пока еще не проверял, но под Linux компилировал.
Все мои изменения в MR.

[kuzulis]

2 b-s-a

Спасибо огромное.
Я вот жду жду пока кто-то покритикует и наставит на путь истинный,
а то что-то зачумел совсем

1. Считаю, что методы SerialPortPrivate::setReadNotificationEnabled и пр. следует перенести в AbstractSerialPortPrivate. В том числе и нотификатор, дабы не плодить сущностей.

Ок, вечером сделаем. Я и сам думал об этом.

2. Считаю, что SerialPort не должен вызывать приватные/защищенные методы SerialPortPrivate/AbstractSerialPortPrivate (начал работать с этим, но лучше пусть автор сделает)

Согласен.
Т.е. типа их (эти приватные/защищенные методы) сделать открытыми?
Но лучше примерчик замерж.

3. serialport_p.h, думаю, стоит разделить по архитектурам. Не нравятся мне такие нагромождения.

Ок. Мне тоже уже не нравится.
Вопросы:
При раскидке по архитектурам, имя класса SerialPortPrivate оставлять таким же или что-то типа WinSerialPortPrivate, UnixSerialPortPrivate, и т.п. ?
И если имя менять - то как быть с конструкторами SerialPort:

SerialPort::SerialPort(QObject *parent)
: QIODevice(parent)
, d_ptr(new SerialPortPrivate(this)) // << вот с этим именем SerialPortPrivate как быть тогда?
{
}

Все мои изменения в MR.

Ок, посмотрел, вечером объединю всё.

[kuzulis]

1. Считаю, что методы SerialPortPrivate::setReadNotificationEnabled и пр. следует перенести в AbstractSerialPortPrivate. В том числе и нотификатор, дабы не плодить сущностей.

Ок, вечером сделаем. Я и сам думал об этом.

Нет, нифига при таком подходе не получается.
ИМХО, все-таки нужно было делать через промежуточный класс engine

2 b-s-a
Также глянул твой цикл где ты применяешь бинарный поиск и заметил, что если скорость будет равна 4000000 бод, то она в
список не добавится, т.к. её адрес равен standardRates_end.
Нужно наверное в массив еще добавить любое значение больше чем 4000000 - и тогда должно работать. Не?

[b-s-a]

С именами варианта 2:
1. Для всех архитектур одно имя
2. Для каждой архитектуры свое имя, но в конце хидера typedef на общее имя. typedef лучше стоит делать в общем хидере, который будет условно подключать нужный платформенный.

Нет, нифига при таком подходе не получается.
ИМХО, все-таки нужно было делать через промежуточный класс engine

Почему не получается? Сделай их абстрактными, а в SerialPortPrivate переопредели. Какие проблемы?

[kuzulis]

Почему не получается? Сделай их абстрактными, а в SerialPortPrivate переопредели. Какие проблемы?

С нотификатором хрень получается. Хотя.. хз..

Не, переопределять

    bool isReadNotificationEnabled() const;
    void setReadNotificationEnabled(bool enable);
    bool isWriteNotificationEnabled() const;
    void setWriteNotificationEnabled(bool enable);

скорее всего не придется, т.к. у этих методов одна и таже реализация.


ЗЫ:
И глянь выше мое сообшение (я отредактировал) про бинарный поиск.

[b-s-a]

Также глянул твой цикл где ты применяешь бинарный поиск и заметил, что если скорость будет равна 4000000 бод, то она в
список не добавится, т.к. её адрес равен standardRates_end.
Нужно наверное в массив еще добавить любое значение больше чем 4000000 - и тогда должно работать. Не?

Именно что "не". Смотри, если у тебя массив из N элементов, то что лежит по индексу N (ведь *(array + N) - это аналог array[N])?

[kuzulis]

>Именно что "не". Смотри, если у тебя массив из N элементов, то что лежит по индексу N (ведь *(array + N) - это аналог array[N])?
Да, точно, по индексу N переменная будет за пределом (N-1) массива лежать и в ней будет всякая хрень. Тупил ночью  

Но с архитектурой приватных классов библиотеки срочно нужно что-то предпринять/переделать. А то какая то хрень получилась.
Но вот что - хз.

[b-s-a]

И какая же?

[kuzulis]

И какая же?

ХЗ какая, но я вот предлагаю теперь такую, с учетом твоих пожеланий о разделении по платформам.
Я приаттачил простенький проектик с наброском новой архитектуры.

Там изменения коснулись только приватных классов SerialPortPrivate и SerialPortNotifier.
Теперь классы SerialPortPrivate и  SerialPortNotifier являются абстрактными, и имеют статический метод create(),
который создает наследников SerialPortPrivate и SerialPortNotifier в зависимости от платформы.

т.е. наследниками являются WinSerialPortPrivate, UnixSerialPortPrivate  и т.п., WinSerialPortNotifier , UnixSerialPortNotifier и т.п.
и, соответственно, теперь в конструкторе SerialPort вместо

Код

C++ (Qt)
SerialPort::SerialPort(QObject *parent)
    : QIODevice(parent)
    , d_ptr(new SerialPortPrivate(this))
{}
 

теперь

Код

C++ (Qt)
SerialPort::SerialPort(QObject *parent)
    : QObject(parent)
    , d_ptr(SerialPortPrivate::create(this))
{
}
 

В общем, зацени наметаным глазом, если будет время (желательно побыстрее).

[b-s-a]

Можно и так. А можно реализацию этого SerialPortPrivate::create кинуть в платформенные cpp-файлы.

[pastor]

Там изменения коснулись только приватных классов SerialPortPrivate и SerialPortNotifier.
Теперь классы SerialPortPrivate и  SerialPortNotifier являются абстрактными, и имеют статический метод create(),
который создает наследников SerialPortPrivate и SerialPortNotifier в зависимости от платформы.

.е. наследниками являются WinSerialPortPrivate, UnixSerialPortPrivate  и т.п., WinSerialPortNotifier , UnixSerialPortNotifier и т.п.

А почему не выбрали для всех архитектур одно имя? Зачем так сложно: абстрактный класс, наследование, статический метод? API данных класов я так понимаю одинаковое. Разместить объявление в одном общем *.h и разнести определение по *.cpp для каждой платформы: *_win.cpp, *_unix.cpp, *_mac.cpp, etc.

[b-s-a]

А почему не выбрали для всех архитектур одно имя? Зачем так сложно: абстрактный класс, наследование, статический метод? API данных класов я так понимаю одинаковое. Разместить объявление в одном общем *.h и разнести определение по *.cpp для каждой платформы: *_win.cpp, *_unix.cpp, *_mac.cpp, etc.

В принципе, можно и так. Но... У всех приватных классов есть общие данные. Логично их вынести в общий предок, чтобы не дублировать код. А виртуальные методы были сделаны для того, чтобы добавить немного контроля над реализациями...

[pastor]

У всех приватных классов есть общие данные. Логично их вынести в общий предок, чтобы не дублировать код.

Да, с этим согласен. Общие данные нужно вынести конечно

2 b-s-a: А эти данные общие для SerialPortPrivate и SerialPortNotifier или у этих классов свои данные?

[b-s-a]

Notifier, я так понимаю, входит в состав Private в виде члена. Не помню точно, но кажется он платформозависимый.

[kuzulis]

API данных класов я так понимаю одинаковое. Разместить объявление в одном общем *.h и разнести определение по *.cpp для каждой платформы: *_win.cpp, *_unix.cpp, *_mac.cpp, etc.

так оно и было (есть)

Notifier, я так понимаю, входит в состав Private в виде члена. Не помню точно, но кажется он платформозависимый.

Именно.

Ну так что, годится новая структура?