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

Return the actual default partition in dds_qget_partition() whenever possible #1824

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TheFixer
Copy link
Contributor

@TheFixer TheFixer commented Sep 7, 2023

I have run into an issue related to dds_qget_partition() a number of times. Just recently it reached my annoyance threshold level, so I decided to make a pull request for it.

The issue has to do with dds_qget_partition() related to the default partition. Currently, the behaviour of dds_qget_partition() is to return zero as the number of partition and a NULL-ish list of partitions in case the partition is the default partition (i.e., ""). According to the DDS specification is perfectly legitimate, so from a functional point of view I have no problem with this whatsoever.

However, from a useability point of view I do have problem with it. Typically, in application code that needs to access the code you do something like this:

if (dds_qget_partition(qos, &n, &partitions)) {
  for (int i=0; i < n; i++) {
    do_something(partition[i]);
  }
}

For the default partition this doesn't work, because the default partition is treated differently: it returns n=0 in that case, so it suggests that there are no partition at all! As a consequence, an application developer needs to treat the default partition case as a special case, in order to deal with the default partition. This is a pity, because it complicates code writing for an application deveoper. And it is easy to forget that the default partition should be treated differently.

To makes life for application developers a little bit easier I suggest to treat the default partition case similar as normal partitions whenever possible. That means that when dds_qget_partition(qos, &n, &partitions) is called and the default partition must be returned, then it returns n=1 and partitions[0]="" for the default partition (provided &n and &partitions are non-NULL). In this way an application developer can handle the default partition that is returned from dds_qget_partition() similar as partitions.

This pull request contains the fix that changes the behaviour of dds_qget_partition() for the default partition. In case you share my viewpoints regarding this issue, I would appreciate if you can have a look at this pull request.

Copy link
Contributor

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

I somehow do have a recollection of having seen this a long time ago (that is consistent with the date you created the PR ...) but somehow I never ended up doing the review.

The only downside that I can see is that this means

dds_qset_partition (q, 0, NULL);
dds_qget_partition (q, &n, &ps);

would transform the (0, NULL) into (1, ""), and that in itself could be construed as surprising.

In my view, the primary purpose of qget is to inspect the QoS, not to do this type of round-trip (and in any case, the meaning of the two is the same), and I agree that the (0, NULL) is somewhat of a pitfall for applications. I think returning (1, "") indeed improves the API.

The documentation also does not give a hint to the possibility of getting (0, NULL) out. It would be wise to also update the documentation to precisely specify the behaviour.

This all being such a long time ago ... if you don't want to make these changes, I'll do them in a separate commit.

* actually return the default partition (i.e.,
* the empty string "").
* */
if (n) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 615 & 616 ensure (ps ≠ NULL) ⇒ (n ≠ NULL), so we are certain to never end up in the else case. I think the comment that says "this is the default partition" might better be rephrased as "this implies the default partition", then the remainder of the comment, the if (n) and the else branch can be thrown out.

@@ -618,8 +618,20 @@ bool dds_qget_partition (const dds_qos_t * __restrict qos, uint32_t *n, char ***
*n = qos->partition.n;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the behaviour that, for an entity created with an empty set of partitions:

dds_qget_partition (q, &nA, NULL);
dds_qget_partition (q, &nB, &ps);
assert (nA == nB);

will fail.

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