Adding exception type for empty callback functions (reopening of PR #301)#304
Conversation
This exception will be thrown when an empty std::function is passed to the callback connection module. Adding the type first so that tests can be written.
Some L2 tests were asserting that the std::bad_function_call exception should be thrown when a bad callback is called. We will be changing that behavior so that the exception is thrown when the callback connection is established, requiring updated test cases.
The CalleeHandle class is the "owner" of the callback function objects associated with a callback connection. As such, it will check the validity of those function objects and throw the EmptyFunctionObject exception if either the callback or cleanup function is empty / invalid.
lukas-he
changed the title
PLeVasseur
left a comment
PLeVasseur
left a comment
There was a problem hiding this comment.
Not much to say! LGTM. I learned a bit more C++ reading through here.
I saw that a couple of stages of CI fell over. @lukas-he -- would you like to investigate?
|
I looked into the error and as far as I understand, the linting fails before actually linting, but when trying to install clang-tidy-12 see here. So we could go to a newer clang-tidy version. What do you think @PLeVasseur? |
Co-authored-by: Pete LeVasseur <plevasseur@gmail.com>
Seems like a reasonable solution if the previous version we're using is not available and doesn't in-turn flag a bunch of more things to fix 😅 IIRC the default runner image is now Ubuntu 24.04.1, see here. Likely that version of clang-tidy may not be available. If you find that updating clang-tidy flags a bunch of additional things to fix, one option is to use Ubuntu 22.04 for now. Then address the issues found by the new clang-tidy all in one PR. It's a thought! |
…cks-ci specify clang version 17 in ci
|
I went with clang-tidy 17. In my fork, the lint jobs worked so I integrated clang-tidy 17 here. @PLeVasseur could you please approve the CI here. |
|
@lukas-he -- is the idea that we get eclipse-uprotocol/up-conan-recipes#32 merged, then try to merge this one? Wondering if there's anything we'd see that differs enough that up-cpp would remain broken? 🤔 If so, I suppose I'd like to have a set of patches / PRs which update things to a working state (with proof of this with a dummy PR branch), but land them one by one. What do you think about this approach? |
I ran both clang-tidy-12 and clang-tidy-18. Interestingly, the number of warnings is actually almost the same, but the linters detect very different issues (see the occurence counts below if you're curious). I suggest we go with your suggestion of pinning the Ubuntu 22.04 image in CI and using clang-tidy-12 for now. |
Sounds good as a starting point. |
|
Code coverage report is ready! 📈
|
PLeVasseur
left a comment
PLeVasseur
left a comment
There was a problem hiding this comment.
Thanks for the contribution @lukas-he!
4 participants
Hello everyone,
As discussed with Greg yesterday, I am reopening a PR for Greg's bugfix contribution with PR #301 .