Add == and != operators to KIO::UDSEntry
ClosedPublic

Authored by meven on Sep 2 2019, 10:03 AM.

Details

Summary

I felt it was missing.

Test Plan

ctest

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16037
Build 16055: arc lint + arc unit
meven created this revision.Sep 2 2019, 10:03 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 2 2019, 10:03 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Sep 2 2019, 10:03 AM
ngraham added a subscriber: ngraham.Sep 3 2019, 1:01 AM

I know it seems silly for such tiny functions, but can you follow the pattern and keep only the function definitions in the header file and move the actual logic into the .cpp file?

src/core/udsentry.h
130

as other -> as the other

131

Just 5.62 (Frameworks don't have minor versions)

139

ditto

140

ditto

pino added a subscriber: pino.Sep 3 2019, 4:38 AM

Please move the implementations in the cpp file, otherwise it will be impossible to change/fix the implementation later on in a binary compatible way.

Also:

  • both the operators ought to be const, since they do not mutate the object (and otherwise they cannot be used to compare const objects)
  • please fix the style in the declarations: const UDSEntry& other -> const UDSEntry &other
pino added a comment.Sep 3 2019, 4:39 AM

Oh, and also please add tests for them in UDSEntryTest.

meven updated this revision to Diff 65295.Sep 3 2019, 9:11 AM

Move impl to cpp file, fix comment, add unit test

meven updated this revision to Diff 65296.Sep 3 2019, 9:13 AM

Clean spaces changes

meven updated this revision to Diff 65297.Sep 3 2019, 9:19 AM

Fix test udsentrybenchmark

meven edited the test plan for this revision. (Show Details)Sep 3 2019, 9:19 AM
meven edited the test plan for this revision. (Show Details)
meven updated this revision to Diff 65298.Sep 3 2019, 9:22 AM

fix since mention

ngraham accepted this revision.Sep 3 2019, 12:43 PM
This revision is now accepted and ready to land.Sep 3 2019, 12:43 PM
pino added a comment.Sep 3 2019, 10:02 PM

please use QCOMPARE/QVERIFY instead of Q_ASSERT in QTest tests

autotests/udsentrytest.cpp
301

typo, "additional"

meven updated this revision to Diff 65607.Sep 7 2019, 9:29 PM

Fix test and implementation

meven updated this revision to Diff 65608.Sep 7 2019, 9:30 PM
meven marked 5 inline comments as done.

typo fix

meven updated this revision to Diff 65705.Sep 9 2019, 9:19 PM

Remove unneeded friend function declarations

meven updated this revision to Diff 66086.Sep 14 2019, 7:56 PM

Update @since references

meven added a comment.Sep 14 2019, 7:56 PM

@pino if this is fine for you...

dfaure accepted this revision.Sep 20 2019, 12:37 PM
dfaure added a subscriber: dfaure.

Looks ok, but I'm just curious about the use case. "I felt it was missing" doesn't sound as strong an argument as "I need this"...

Slaves are supposed to mostly create those, not compare them, and apps are supposed to use KFileItem rather than UDSEntry directly.

meven added a comment.Sep 20 2019, 6:52 PM

Looks ok, but I'm just curious about the use case. "I felt it was missing" doesn't sound as strong an argument as "I need this"...

Slaves are supposed to mostly create those, not compare them, and apps are supposed to use KFileItem rather than UDSEntry directly.

The use case I felt I might need it, is when running stats for a file which I already stated earlier and have a KFileItem already.
And checking if the file changed between the two stats.
Without this it would be hard to do.

Using the entry to compare the KFileItem seemed to me the simplest course of action (KFileItem equals compares only file path) but lacked the equal operators.

Note that there is KFileItem::cmp for a more thorough comparison of two fileitems. But it only looks at some UDS fields -- those that matter at the KDirLister level.
Or maybe it shows that UDSEntry::operator== was missing so KFileItem::cmp uses a poor man's version of that ;)
This confirms the need for such an operator== indeed (though I wouldn't change the implementation of KFileItem::cmp until we see a need to do that).

Thanks for the explanation.

This revision was automatically updated to reflect the committed changes.