kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.
This definitely needs another revision. Please fix the outstanding issues.
INLINE COMMENTS
> iokitblock.h:43
> +
> + virtual int deviceMajor() const;
> + virtual int deviceMinor() const;
Here and below: Missing `Q_DECL_OVERRIDE`
> iokitdevice.cpp:151
> + size_t size = 0;
> + if (sysctlbyname("hw.model", NULL, &size, NULL, 0) == 0 && size > 0) {
> + model = new char [size];
Use `nullptr` everywhere
> iokitdevice.cpp:154
> + if (sysctlbyname("hw.model", model, &size, NULL, 0) == 0) {
> + qModel= QLatin1String(model);
> + }
Style: Use `qModel = ...`
> iokitdevice.cpp:156
> + }
> + delete model;
> + }
`new`/`delete` mismatch. Use `delete[]`
> iokitdevice.cpp:167
> + {
> + type << Solid::DeviceInterface::Unknown;
> + }
Initialize at class member decl
> iokitdevice.cpp:171
> + {
> + if (parent) {
> + delete parent;
That's *very* odd style. Why does a private class delete its public
counterpart? I've never seen this.
> iokitdevice.cpp:197
> + type = typesFromEntry(entry, properties, mainType);
> +// if (udi.contains(QStringLiteral("IOBD")) ||
> udi.contains(QStringLiteral("BD PX"))) {
> +// qWarning() << "Solid: BlueRay entry" << entry << "mainType=" <<
> mainType << "typeList:" << type
Don't leave commented code around. Either enable this code paths properly via
categorized logging or remove it altogether.
> iokitopticaldrive.cpp:211
> + }
> + delete ioDVDServices;
> + d = new Private(device, devCharMap);
Create on `ioDVDServices` on the stack to begin with?
> iokitopticaldrive.cpp:333
> +{
> + QList<int> speeds;
> + return speeds;
`return {}`
> iokitopticaldrive.cpp:354
> + return true;
> + } else {
> + emit ejectDone(Solid::ErrorType::OperationFailed, QVariant(),
> m_device->udi());
Coding style: Would turn around this two branches (and remove the else part) to
make the intended meaning more clear:
Pseudo code:
if (errorCase) {
return ...;
}
// normal case, code flow continues
return ...;
That's usually the common style
> iokitprocessor.cpp:84
> +
> +const QString Processor::vendor()
> +{
Remove `const`
> iokitprocessor.cpp:87
> + QString qVendor = QString();
> + char *vendor = NULL;
> + size_t size = 0;
Way too much error prone `new`/`delete` logic on naked char arrays. Every
invocation here is wrong (causing undefined behavior) due to the
`new[]`/delete` mismatch mentioned above.
Factor that out reading into a `QString` into a separate function, cf.
https://github.com/trueos/lumina/blob/master/src-qt5/core/libLumina/LuminaOS-DragonFly.cpp#L29
and use it everywhere instead.
> iokitprocessor.cpp:99
>
> +const QString Processor::product()
> +{
Remove `const`
> iokitstorage.cpp:36
> + {
> + daRef = 0;
> + daDict = 0;
Here and one line below: Could be initialized at decl again.
> iokitstorage.cpp:36
> + {
> + daRef = 0;
> + daDict = 0;
`nullptr`
> iokitstorage.cpp:51
> + CFRelease(daDict);
> + daDict = 0;
> + }
`nullptr`, etc. pp.
> iokitstorage.cpp:75
> +
> +IOKitStorage::IOKitStorage(const IOKitDevice *device)
> + : Block(device)
Please just remove the constructors taking a `const IOKitDevice *device` and
adapt uses (just use `IOKitDevice *device` everywhere). It's unusual for
pointer types to be `const` in such contexts. It just adds noise.
> iokitstorageaccess.cpp:41
> + } else {
> + daRef = 0;
> + }
Here and below: `nullptr`
> iokitstorageaccess.cpp:91
> +{
> + // mount points can change (but can they between invocations of
> filePath()?)
> + if (d->daRef) {
Early-return would reduce the indentation level here [1].
if (errorCase)
// return early
return {};
}
// normal case
// ...
[1]
https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement
> iokitstorageaccess.cpp:132
> + return false;
> + } else {
> + // TODO?
Coding style: Eliminate `else` branch, just `return in function-level scope.
> iokitstorageaccess.cpp:142
> + return false;
> + } else {
> + // TODO?
Dito
> iokitvolume.cpp:42
> + } else {
> + daRef = 0;
> + }
Here and below: `nullptr`
> iokitvolume.cpp:121
> + if (dict) {
> +// qWarning() << Q_FUNC_INFO;
> +// CFShow(dict);
Remove commented code.
> iokitvolume.cpp:177
> + if (d->daRef) {
> + CFDictionaryRef dict = DADiskCopyDescription(d->daRef);
> + if (dict) {
That's a lot of copy-pasted code amongst `fsType`, `label`, `vendor`, `product,
`description`. I'm sure that can be done better.
> iokitvolume.h:45
> +
> + virtual bool isIgnored() const;
> + virtual Solid::StorageVolume::UsageType usage() const;
Here and below: Missing `Q_DECL_OVERRIDE` again
> iokitvolume.h:53
> +
> + const QString vendor();
> + const QString product();
Here and below: No `const` for return types on implicitly-shared types (just
adds noise)
The functions can be made `const` though.
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D7401
To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac
