[Inotify] Remove dead/duplicate code
ClosedPublic

Authored by bruns on Mar 26 2020, 11:45 PM.

Details

Summary

event->mask & EventQueueOverflow is already checked once early in the
function, no need to do the check twice.

Diff Detail

Repository
R293 Baloo
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bruns created this revision.Mar 26 2020, 11:45 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptMar 26 2020, 11:45 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bruns requested review of this revision.Mar 26 2020, 11:45 PM
bruns added inline comments.Mar 26 2020, 11:49 PM
src/file/kinotify.cpp
350 ↗(On Diff #78600)

First check is here

pino added a subscriber: pino.Mar 26 2020, 11:55 PM
pino added inline comments.
src/file/kinotify.cpp
350 ↗(On Diff #78600)

... but only also if wd is < 0

bruns marked an inline comment as done.Mar 27 2020, 12:21 AM
bruns added inline comments.
src/file/kinotify.cpp
350 ↗(On Diff #78600)

man inotify

IN_Q_OVERFLOW
Event queue overflowed (wd is -1 for this event).

bruns marked an inline comment as done.Mar 27 2020, 12:29 AM
pino added inline comments.Mar 27 2020, 8:57 AM
src/file/kinotify.cpp
350 ↗(On Diff #78600)

then add an assert for this case, referencing the documentation?

bruns marked an inline comment as done.Mar 27 2020, 5:26 PM
bruns added inline comments.
src/file/kinotify.cpp
350 ↗(On Diff #78600)

asserts are IMHO pointless, nobody enables them, but it clutters the code. Anyone touching the code should and has to read the man pages etc anyway.

pino added inline comments.Mar 27 2020, 5:35 PM
src/file/kinotify.cpp
350 ↗(On Diff #78600)

no, an assert is sort of "program by contract": you make it clear that this situation ought to not happen, and it if does, at least some people may notice that

Anyone touching the code should and has to read the man pages etc anyway.

and an explicit mention/comment in the code never killed anyone, nor made the code "cluttered"; also what is valid today might change in the future, so writing down why a check/assert was added make sure that people reading the changed documentation know why it made sense when it was added

bruns marked an inline comment as done.Mar 27 2020, 6:02 PM
bruns added inline comments.
src/file/kinotify.cpp
350 ↗(On Diff #78600)

After two years of maintaining baloo (and other software for much longer) I can assure you you are wrong.

If we checked all the conditions which are guaranteed by the API (null termination of strings, only complete events, bounds check of event->len ...) here and everywhere else, the could would be cluttered. And just picking an arbitrary condition to be more equal is nonsense.

This is kernel API, it does not change.

Comments are useful when some code is not obvious. Copying an explicitly spelled out condition from the man page does clutter the code. Otherwise we would have to copy the whole man page to have the code covered in its entirety.

bruns marked 5 inline comments as done.Apr 2 2020, 10:02 AM
ngraham accepted this revision.Apr 7 2020, 3:05 PM

I think this is fine.

This revision is now accepted and ready to land.Apr 7 2020, 3:05 PM
This revision was automatically updated to reflect the committed changes.