KSANECore in KDE review

[Albert Astals Cid]

El diumenge, 27 de març de 2022, a les 18:29:18 (CEST), Alexander Stippich va escriure:
> Hello everyone,
> 
> KSANECore is now in KDE review. Kåre and I mentioned it in previous emails 
> before, but as a short summary:
> KSANECore is a Qt interface to the SANE scanner library. It is stripped out of 
> the KSaneWidget of libksane without any QWidget dependency. It is currently  
> located inside the libksane repository as KSaneCore and basically just copied 
> into the new repository.
> 
> Due to breaking API anyway, the code was cleaned up, better named as well as 
> smaller API fixes made on top. Also, KSANECore is fully REUSE compliant.
> KSaneWidget of libksane will remain ABI compatible.
> 
> I don't know if it is strictly required to move the new repo with already 
> existing code through KDE review, but I guessed it is better to be on the safe 
> side :) 


Ran it through clang-tidy + my preferred options, came with a few suggestions, nothing earth shattering but i think it makes sense, if you don't, that's fine, attached the file with the results, feel free to ask if you don't understand what some warnings say


The only other question i have is if you think that it may make sense to hide the members of DeviceInfo behind a d-pointer in case it ever needs more and that would be an ABI change (not sure which kind of ABI promises you want to have for this library).


Ok, I lied, I have another question, what's the goal for the library? independently released? KDE Gear? KDE Frameworks?

Cheers,
  Albert

> 
> The plan is to switch libksane and Skanpage over to the new library during the 
> KDE Gear 22.08 release cycle. The adaptions are located at
> https://invent.kde.org/astippich/skanpage/-/commits/ksanecore
> and
> https://invent.kde.org/astippich/libksane/-/commits/ksanecoreSplit
> 
> Best regards,
> Alex
> 
> 
> 
> 

-------------- next part --------------
ksanecore/src/coreinterface.h:113:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors]
    CoreInterface(QObject *parent = nullptr);
    ^
    explicit

ksanecore/src/scanthread.h:43:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors]
    ScanThread(SANE_Handle handle);
    ^
    explicit

ksanecore/src/coreinterface_p.h:35:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors]
    CoreInterfacePrivate(CoreInterface *parent);
    ^
    explicit

ksanecore/src/coreoption.h:73:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors]
    CoreOption(QObject *parent = nullptr);
    ^
    explicit

ksanecore/src/internaloption.h:22:5: error: constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors]
    InternalOption(BaseOption *option, QObject *parent = nullptr);
    ^
    explicit




ksanecore/src/coreinterface.cpp:35:26: error: use std::make_unique instead [modernize-make-unique,-warnings-as-errors]
    : QObject(parent), d(std::unique_ptr<CoreInterfacePrivate>(new CoreInterfacePrivate(this)))
                         ^~~~~~~~~~~~~~~                       ~~~~~~~~~~~~~~~~~~~~~~~~~    ~
                         std::make_unique

ksanecore/src/coreoption.cpp:16:62: error: use std::make_unique instead [modernize-make-unique,-warnings-as-errors]
CoreOption::CoreOption(QObject *parent) : QObject(parent), d(std::unique_ptr<OptionPrivate>(new OptionPrivate()))
                                                             ^~~~~~~~~~~~~~~                ~~~~~~~~~~~~~~~~~~~
                                                             std::make_unique




ksanecore/src/coreinterface.cpp:133:98: error: the parameter 'userName' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
CoreInterface::OpenStatus CoreInterface::openRestrictedDevice(const QString &deviceName, QString userName, QString password)
                                                                                                 ^
                                                                                         const  &

ksanecore/src/coreinterface.cpp:133:116: error: the parameter 'password' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
CoreInterface::OpenStatus CoreInterface::openRestrictedDevice(const QString &deviceName, QString userName, QString password)
                                                                                                                   ^
                                                                                                           const  &




ksanecore/src/coreinterface.cpp:278:5: error: use range-based for loop instead [modernize-loop-convert,-warnings-as-errors]
    for (int i = 0; i < d->m_optionsList.size(); i++) {
    ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        (auto i : d->m_optionsList)

ksanecore/src/authentication.cpp:107:5: error: use range-based for loop instead [modernize-loop-convert,-warnings-as-errors]
    for (int i = 0; i < list.size(); i++) {
    ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        (const auto & i : list)





ksanecore/src/coreinterface.h:213:9: error: function 'KSane::CoreInterface::setOptionsMap' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name,-warnings-as-errors]
    int setOptionsMap(const QMap <QString, QString> &options);
        ^
ksanecore/src/coreinterface.cpp:288:20: note: the definition seen here
int CoreInterface::setOptionsMap(const QMap <QString, QString> &opts)
                   ^
ksanecore/src/coreinterface.h:213:9: note: differing parameters are named here: ('options'), in definition: ('opts')
    int setOptionsMap(const QMap <QString, QString> &options);
        ^                                            ~~~~~~~
                                                     opts