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

Add explicit keyword to constructors #281

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thekurtovic
Copy link
Contributor

@thekurtovic thekurtovic commented Jan 4, 2025

Resolves cppcheck warnings noExplicitConstructor, useInitializationList, and passedByValue.

Resolves cppcheck warnings noExplicitConstructor, and useInitializationList.
@thekurtovic
Copy link
Contributor Author

thekurtovic commented Jan 4, 2025

There's one warning I wasn't able to resolve.
NimBLEUUID(const ble_uuid_any_t& uuid);

If I make that constructor explicit then I get this compile error.

Log
managed_components/esp-nimble-cpp/src/NimBLERemoteValueAttribute.h: In constructor 'NimBLERemoteValueAttribute::NimBLERemoteValueAttribute(const ble_uuid_any_t&, uint16_t)':
managed_components/esp-nimble-cpp/src/NimBLERemoteValueAttribute.h:189:107: error: no matching function for call to 'NimBLEAttribute::NimBLEAttribute(const ble_uuid_any_t&, uint16_t&)'
  189 |     NimBLERemoteValueAttribute(const ble_uuid_any_t& uuid, uint16_t handle) : NimBLEAttribute(uuid, handle) {}
      |                                                                                                           ^
In file included from managed_components/esp-nimble-cpp/src/NimBLERemoteService.h:24,
                 from managed_components/esp-nimble-cpp/src/NimBLEDevice.h:261:
managed_components/esp-nimble-cpp/src/NimBLEAttribute.h:48:5: note: candidate: 'NimBLEAttribute::NimBLEAttribute(const NimBLEUUID&, uint16_t)'
   48 |     NimBLEAttribute(const NimBLEUUID& uuid, uint16_t handle) : m_uuid{uuid}, m_handle{handle} {}
      |     ^~~~~~~~~~~~~~~
managed_components/esp-nimble-cpp/src/NimBLEAttribute.h:48:39: note:   no known conversion for argument 1 from 'const ble_uuid_any_t' to 'const NimBLEUUID&'
   48 |     NimBLEAttribute(const NimBLEUUID& uuid, uint16_t handle) : m_uuid{uuid}, m_handle{handle} {}
      |                     ~~~~~~~~~~~~~~~~~~^~~~
managed_components/esp-nimble-cpp/src/NimBLEAttribute.h:29:7: note: candidate: 'constexpr NimBLEAttribute::NimBLEAttribute(const NimBLEAttribute&)'
   29 | class NimBLEAttribute {

Also NimBLEHIDDevice needs to be updated to be compliant with this change.
For example: m_deviceInfoSvc = server->createService(deviceInfoSvcUuid);

Do you think it's better to just create a new constructor which can take a uint16_t, or change such instances like this?
m_deviceInfoSvc = server->createService(NimBLEUUID(deviceInfoSvcUuid));

I've gone with the latter (though I excluded the file from the commit for now), though adding the new constructor would probably make it easier to read NimBLEHIDDevice.

Edit: Or this

static constexpr uint16_t deviceInfoSvcUuid = 0x180a;
static const NimBLEUUID deviceInfoSvcUuid = NimBLEUUID((uint16_t)0x180a);

@h2zero
Copy link
Owner

h2zero commented Jan 6, 2025

I think m_deviceInfoSvc = server->createService(NimBLEUUID(deviceInfoSvcUuid)); is probably fine here as it's an internally managed class so application code isn't affected, still quite readable as well.

@thekurtovic
Copy link
Contributor Author

Would this be considered a breaking change? Any applications making use of these implicit conversions will fail to compile, though the changes should be easy to make.
I'll make a separate PR for NimBLEHIDDevice.

@h2zero
Copy link
Owner

h2zero commented Jan 6, 2025

It might be, depending on the application.

I would suggest adding the HID changes to this PR so we can test the builds at least. Maybe add some more build tests to find where it may fail so it can be documented?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants