Skip to content

feat: add timeout parameter to xml2cpp codegen tool (#509)#510

Open
pavelkaryukov wants to merge 1 commit intoKistler-Group:masterfrom
pavelkaryukov:timeout_as_argument
Open

feat: add timeout parameter to xml2cpp codegen tool (#509)#510
pavelkaryukov wants to merge 1 commit intoKistler-Group:masterfrom
pavelkaryukov:timeout_as_argument

Conversation

@pavelkaryukov
Copy link

A timeout can be added to the XML file using the annotation , where 500ms is the default parameter value required for interoperability with existing APIs.

These changes will allow flexible timeout configuration using configuration files.

@pavelkaryukov
Copy link
Author

The changes allow generating functions that include timeout parameters, enabling runtime timeout configuration

…](https://github.com/pavelkaryukov/sdbus-cpp/issues/509))

A timeout can be added to the XML file using the annotation <annotation name="org.freedesktop.DBus.Method.Timeout" value="500ms"/>,
where 500ms is the default parameter value required for interoperability with existing APIs.

These changes will allow flexible timeout configuration using configuration files.
@sangelovic
Copy link
Collaborator

@pavelkaryukov Happy New Year! I finally found time to review the PR, thanks for your patience. However, it took me a while to understand the logic. The proposed solution has several drawbacks:

  • there is unclear, not immediately revealing connection between timeout annotation and generating an extra timeout parameter (which I find confusing),
  • the behavior of timeout parameter generation gets activated only when timeout annotation is present (which means different thing),
  • timeout parameter has a fixed name given by the generator, which may easily collide with already existing name given by the client.

So I cannot accept the proposed solution. What we can do instead is to decouple from the existing timeout annotation and create a new one (like TimeoutParam or so), which

  • either picks a parameter from the arg list and uses it for generating withTimeout() call instead of passing it as a real parameter to the call, or
  • defines the name of the timeout parameter to be appended to the function signature, so the client has control over the name (and if timeout annotation is present as well, its value may be taken as well as the default parameter value).

I find the first one nicely generic (timeout parameter can be anywhere in the function signature), but the second one seems sufficient for this special use case and quite simple to implement in this PR -- we just need an additional annotation TimeoutParam whose value will be the name of the added parameter.

Do you have any additional comments. Will you please do the requested changes?

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