Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand use of type-safe BitFlags and enum class. #5232

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

kpschoedel
Copy link
Contributor

Problem

  • BitFlags.h has relatively unsafe deprecated functions.
  • Some bit flags use enum rather than type-safe enum class.

Summary of Changes

  • Removed deprecated SetFlags(), ClearFlags(), and GetFlags(),
    replacing uses with BitFlags.

  • BitFlags.h changes:

    • Default to use std::underlying_type rather than requiring it as a template parameter.
    • Added BitFlags operations to reduce uses of type-unsafe Raw() and SetRaw():
      • Multi-argument constructors.
      • operator FlagsEnum().
      • ClearAll().
      • HasAny() and HasAll(), following the pattern of the existing HasOnly().
      • Binary &.
  • Converted various bit flag types from enum to enum class:

    • BLEEndPoint::ConnectionStateFlags
    • BLEEndPoint::TimerStateFlags
    • BLEManagerImpl::Flags
    • Command::CommandPathFlags
    • Encoding::HexFlags
    • GenericConfigurationManagerImpl::Flags
    • GenericConnectivityManagerImpl_Thread::Flags
    • GenericConnectivityManagerImpl_WiFi::ConnectivityFlags
    • bdx::RangeControlFlags
    • bdx::StatusCode
    • bdx::TransferControlFlags
    • bdx::TransferRole

#### Problem

- `BitFlags.h` has relatively unsafe deprecated functions.
- Some bit flags use `enum` rather than type-safe `enum class`.

#### Summary of Changes

- Removed deprecated `SetFlags()`, `ClearFlags()`, and `GetFlags()`,
  replacing uses with `BitFlags`.

- BitFlags.h changes:
  - Default to use `std::underlying_type` rather than requiring it as a template parameter.
  - Added `BitFlags` operations to reduce uses of type-unsafe `Raw()` and `SetRaw()`:
    - Multi-argument constructors.
    - `operator FlagsEnum()`.
    - `ClearAll()`.
    - `HasAny()` and `HasAll()`, following the pattern of the existing `HasOnly()`.
    - Binary `&`.

- Converted various bit flag types from `enum` to `enum class`:
  - BLEEndPoint::ConnectionStateFlags
  - BLEEndPoint::TimerStateFlags
  - BLEManagerImpl::Flags
  - Command::CommandPathFlags
  - Encoding::HexFlags
  - GenericConfigurationManagerImpl::Flags
  - GenericConnectivityManagerImpl_Thread::Flags
  - GenericConnectivityManagerImpl_WiFi::ConnectivityFlags
  - bdx::RangeControlFlags
  - bdx::StatusCode
  - bdx::TransferControlFlags
  - bdx::TransferRole
*
* @param other Flag(s) to set. Any flags not set in @a other are unaffected.
*/
BitFlags & Set(const BitFlags & other)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we implement operator |= and | instead?

I find declaring BitFlags for single constants a bit odd:

// why would we need this:
static const BitFlags<CertValidateFlags> sIgnoreNotBeforeFlag(CertValidateFlags::kIgnoreNotBefore);
static const BitFlags<CertValidateFlags> sIgnoreNotAfterFlag(CertValidateFlags::kIgnoreNotAfter);

// when we can just use 
// CertValidateFlags::kIgnoreNotBefore
// CertValidateFlags::kIgnoreNotAfter)

directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I agree, but this PR already exploded beyond what I thought it would be when I pulled on the loose string, so I drew the line at making the flag enums type-safe using the existing notation.

The TestChipCert usage does seems a bit strange. I assume the idea was to have shorter names to keep sValidationTestCases somewhat readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, Clear would remain a bit ugly — flags &= ~BitFlags<FlagsType>(kFlagValue) — unless we define a free operator~ on enum classes.

@woody-apple woody-apple merged commit 4b8d558 into project-chip:master Mar 9, 2021
@kpschoedel kpschoedel deleted the bitflags branch March 10, 2021 15:10
mspang added a commit that referenced this pull request Mar 11, 2021
mspang added a commit that referenced this pull request Mar 11, 2021
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this pull request Mar 11, 2021
- `BitFlags.h` has relatively unsafe deprecated functions.
- Some bit flags use `enum` rather than type-safe `enum class`.

#### Summary of Changes

- Removed deprecated `SetFlags()`, `ClearFlags()`, and `GetFlags()`,
  replacing uses with `BitFlags`.

- BitFlags.h changes:
  - Default to use `std::underlying_type` rather than requiring it as a template parameter.
  - Added `BitFlags` operations to reduce uses of type-unsafe `Raw()` and `SetRaw()`:
    - Multi-argument constructors.
    - `operator FlagsEnum()`.
    - `ClearAll()`.
    - `HasAny()` and `HasAll()`, following the pattern of the existing `HasOnly()`.
    - Binary `&`.

- Converted various bit flag types from `enum` to `enum class`:
  - BLEEndPoint::ConnectionStateFlags
  - BLEEndPoint::TimerStateFlags
  - BLEManagerImpl::Flags
  - Command::CommandPathFlags
  - Encoding::HexFlags
  - GenericConfigurationManagerImpl::Flags
  - GenericConnectivityManagerImpl_Thread::Flags
  - GenericConnectivityManagerImpl_WiFi::ConnectivityFlags
  - bdx::RangeControlFlags
  - bdx::StatusCode
  - bdx::TransferControlFlags
  - bdx::TransferRole

Reland of 4b8d558 (project-chip#5232) to a logic error in `*/BLEManagerImpl.cpp` and a large
accidental deletion in `K32W/BLEManagerImpl.cpp`.
bzbarsky-apple pushed a commit that referenced this pull request Mar 11, 2021
- `BitFlags.h` has relatively unsafe deprecated functions.
- Some bit flags use `enum` rather than type-safe `enum class`.

#### Summary of Changes

- Removed deprecated `SetFlags()`, `ClearFlags()`, and `GetFlags()`,
  replacing uses with `BitFlags`.

- BitFlags.h changes:
  - Default to use `std::underlying_type` rather than requiring it as a template parameter.
  - Added `BitFlags` operations to reduce uses of type-unsafe `Raw()` and `SetRaw()`:
    - Multi-argument constructors.
    - `operator FlagsEnum()`.
    - `ClearAll()`.
    - `HasAny()` and `HasAll()`, following the pattern of the existing `HasOnly()`.
    - Binary `&`.

- Converted various bit flag types from `enum` to `enum class`:
  - BLEEndPoint::ConnectionStateFlags
  - BLEEndPoint::TimerStateFlags
  - BLEManagerImpl::Flags
  - Command::CommandPathFlags
  - Encoding::HexFlags
  - GenericConfigurationManagerImpl::Flags
  - GenericConnectivityManagerImpl_Thread::Flags
  - GenericConnectivityManagerImpl_WiFi::ConnectivityFlags
  - bdx::RangeControlFlags
  - bdx::StatusCode
  - bdx::TransferControlFlags
  - bdx::TransferRole

Reland of 4b8d558 (#5232) to a logic error in `*/BLEManagerImpl.cpp` and a large
accidental deletion in `K32W/BLEManagerImpl.cpp`.
Damian-Nordic pushed a commit to Damian-Nordic/connectedhomeip that referenced this pull request Mar 18, 2021
- `BitFlags.h` has relatively unsafe deprecated functions.
- Some bit flags use `enum` rather than type-safe `enum class`.

#### Summary of Changes

- Removed deprecated `SetFlags()`, `ClearFlags()`, and `GetFlags()`,
  replacing uses with `BitFlags`.

- BitFlags.h changes:
  - Default to use `std::underlying_type` rather than requiring it as a template parameter.
  - Added `BitFlags` operations to reduce uses of type-unsafe `Raw()` and `SetRaw()`:
    - Multi-argument constructors.
    - `operator FlagsEnum()`.
    - `ClearAll()`.
    - `HasAny()` and `HasAll()`, following the pattern of the existing `HasOnly()`.
    - Binary `&`.

- Converted various bit flag types from `enum` to `enum class`:
  - BLEEndPoint::ConnectionStateFlags
  - BLEEndPoint::TimerStateFlags
  - BLEManagerImpl::Flags
  - Command::CommandPathFlags
  - Encoding::HexFlags
  - GenericConfigurationManagerImpl::Flags
  - GenericConnectivityManagerImpl_Thread::Flags
  - GenericConnectivityManagerImpl_WiFi::ConnectivityFlags
  - bdx::RangeControlFlags
  - bdx::StatusCode
  - bdx::TransferControlFlags
  - bdx::TransferRole

Reland of 4b8d558 (project-chip#5232) to a logic error in `*/BLEManagerImpl.cpp` and a large
accidental deletion in `K32W/BLEManagerImpl.cpp`.
Damian-Nordic pushed a commit to Damian-Nordic/connectedhomeip that referenced this pull request Mar 22, 2021
- Some bit flags use `enum` rather than type-safe `enum class`.

- Removed deprecated `SetFlags()`, `ClearFlags()`, and `GetFlags()`,
  replacing uses with `BitFlags`.

- BitFlags.h changes:
  - Default to use `std::underlying_type` rather than requiring it as a template parameter.
  - Added `BitFlags` operations to reduce uses of type-unsafe `Raw()` and `SetRaw()`:
    - Multi-argument constructors.
    - `operator FlagsEnum()`.
    - `ClearAll()`.
    - `HasAny()` and `HasAll()`, following the pattern of the existing `HasOnly()`.
    - Binary `&`.

- Converted various bit flag types from `enum` to `enum class`:
  - BLEEndPoint::ConnectionStateFlags
  - BLEEndPoint::TimerStateFlags
  - BLEManagerImpl::Flags
  - Command::CommandPathFlags
  - Encoding::HexFlags
  - GenericConfigurationManagerImpl::Flags
  - GenericConnectivityManagerImpl_Thread::Flags
  - GenericConnectivityManagerImpl_WiFi::ConnectivityFlags
  - bdx::RangeControlFlags
  - bdx::StatusCode
  - bdx::TransferControlFlags
  - bdx::TransferRole

Reland of 4b8d558 (project-chip#5232) to a logic error in `*/BLEManagerImpl.cpp` and a large
accidental deletion in `K32W/BLEManagerImpl.cpp`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants