Description of wxMakeGuard (PR #26769)
Yannick Duchêne
I’m not sure about this one.
When I first saw the description of wxMakeGuard, I bugged seeing what was looking like a variadic parameter, not at the end where it should be. To understand, I had a look at the source and saw there are multiple definitions, which explains why the comment mentions overloading.
The change exposes the implementation defined limit. I’m unsure about that, because it seems it is not to be exposed. On an other hand, there is a single implementation, this is not like an interface to various possible implementations. I still left the note about uncertainty, because it may tell their count may vary in future versions.
In the HTML, the explanation appears only for the first definition, but the three other definitions follow immediately after.
The original signature is kept in a comment presenting it as a pseudo‑code illustration, which it was.
In the interface description, the template parameters are typename, while in the source, there are class. May be it makes a difference with dynamic dispatch, I don’t know C++ well enough to tell. I kept what was in the original, typename.
For the return type, I also kept what's in the original, which should be OK, since this type is a base type for the really returned types.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26769
Commit Summary
- e7c367d Description of wxMakeGuard
File Changes
(1 file)
- M interface/wx/scopeguard.h (24)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26769.patch
- https://github.com/wxWidgets/wxWidgets/pull/26769.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26769)I'm not sure why do we need to change this at all, but if we do, I'd rather use vararg template syntax — which is not how it is defined, but close enough and more clear. Could you please change the PR to do this instead?
But it would still be good to know what problem are you trying to fix?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Yannick Duchêne
@Hibou57 pushed 1 commit.
- 222e9ab Variadic template interface for wxMakeGuard
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Yannick Duchêne
Hibou57 left a comment (wxWidgets/wxWidgets#26769)Suggestion applied, and this version is nicer, indeed (originally, I wanted to be as close as possible to the original).
This is not critical. Initially, this was just for declarations to be parsable (or correct), the previous was not. Then this was the idea to have a more easy to understand description.
By the way, the idea of distinct includes for interface documentation, is clever.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26769)Thanks, I'll apply this soon with some minor modifications.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.