Add "Stat" prefix to StatDetails Enum entries
ClosedPublic

Authored by meven on Mar 23 2020, 5:44 PM.

Details

Summary

Relates to D25010

Test Plan

Builds

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24179
Build 24197: arc lint + arc unit
meven created this revision.Mar 23 2020, 5:44 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 23 2020, 5:44 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Mar 23 2020, 5:44 PM
broulik added inline comments.
src/core/global.h
322

Is this change source-compatible?

davidre added inline comments.
src/core/global.h
322

I think this enum was not released yet

meven marked 2 inline comments as done.Mar 24 2020, 8:37 AM
meven added inline comments.
src/core/global.h
322

Indeed it will be part of KF 5.69 (as the comment above the enum tells), view discussion in D25010

meven marked 2 inline comments as done.Mar 24 2020, 8:37 AM
dfaure added inline comments.Mar 24 2020, 7:02 PM
src/core/statjob.cpp
106

This is a weird way of doing this.

A C-style enum is used like KIO::StatBasic.
Here you're using the C++11-class-enum syntax on a C-style enum, which I'm not sure all compilers accept, and which leads to a redundant "Stat".

I suggest to pick one of those two solutions and stick to it:

  1. C-style enum as in this change, but then it's used as KIO::StatBasic etc.
  1. "enum class" and then you *don't* need the Stat prefix in the values, i.e. then you can keep writing KIO::StatDetail::Basic.

The current patch makes it look like StatBasic is in the StatDetail "namespace" when in fact it's not.

meven updated this revision to Diff 78401.Mar 24 2020, 8:06 PM

Properly use KIO::Stat* as C-style enum

meven marked an inline comment as done.Mar 24 2020, 8:07 PM
meven added inline comments.
src/core/statjob.cpp
106

I would rather use C++ enum class but it would require to add some shenanigans since I use currently the enum as a bit map, complicating code and a Q_Flag.
So I cleanup this up to be a plain C-style enum.

dfaure accepted this revision.Mar 24 2020, 8:11 PM

Thanks.

This revision is now accepted and ready to land.Mar 24 2020, 8:11 PM
meven marked an inline comment as done.Mar 24 2020, 8:13 PM

Using enum class would need a bunch of added code such as described http://blog.bitwigglers.org/using-enum-classes-as-type-safe-bitmasks/
I like it a lot more, this adds quite a lot of type safety.

meven added a comment.Mar 25 2020, 7:55 AM

Using enum class would need a bunch of added code such as described http://blog.bitwigglers.org/using-enum-classes-as-type-safe-bitmasks/
I like it a lot more, this adds quite a lot of type safety.

Maybe this could be part of KF6 adding the macros to have enum class used a bitmask and as much as possible enums ported to those enum class.

This revision was automatically updated to reflect the committed changes.