[KIP-848] Tests for: dev_kip848_fix_fast_subscribe_or_unsubscribe

[emasab]

No description provided.

[confluent-cla-assistant]

🎉 All Contributor License Agreements have been signed. Ready to merge.
_{Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.}

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[pranavrth]

No need for FOREACH call here. We are not using topic anywhere below. We can have a simple for loop here.

[pranavrth]

No need for FOREACH here as well. We can just use simple forloop.

[emasab]

don't agree, we have it for every other iterable type, the foreach in general is less typo error prone than writing the initialization and while condition every time. Of course it works only for arrays that are on stack on in structure where we can determine the full size with sizeof() but it's an improvement for these cases.

[pranavrth]

We can keep the definition of RD_ARRAY_FOREACH if you feel like but here the topic is being accessed as topics[i].

If we are keeping RD_ARRAY_FOREACH then we should also have RD_ARRAY_FOREACH_INDEX(ELEM, ARRAY, INDEX) and have RD_ARRAY_FOREACH(ELEM, ARRAY) which internally uses RD_ARRAY_FOREACH_INDEX. We don't need index in FOREACH in general like other high level languages.

[emasab]

It's fine I add them. The index is only for when we need it so it can be a different directive.

[emasab]

if you feel like but here the topic is being accessed as topics[i].

right, I use topic here

[pranavrth]

No need for FOREACH here as simple for loop will work.

[pranavrth]

No need for this foreach as not used in the PR.

[pranavrth]

Few more comments.

[pranavrth]

Let's not make it complex to understand. Remove BLOCK from here. It should be a simple FOREACH.

[emasab]

that way we need to provide the index variable externally instead of defining it here and we need to use always RD_ARRAY_FOREACH_INDEX

[pranavrth]

I think we should keep it simple to understand the functions. We should remove this BLOCK from the macro and make it work it without it. Something like below.

/** Array foreach */
#define RD_ARRAY_FOREACH_INDEX(ELEM, ARRAY, INDEX)                             \
        for ((INDEX = 0, (ELEM) = (ARRAY)[INDEX]);                             \
             INDEX < RD_ARRAY_SIZE(ARRAY);                                     \
             (ELEM) =                                                          \
                 (++INDEX < RD_ARRAY_SIZE(ARRAY) ? (ARRAY)[INDEX] : (ELEM)))

#define RD_ARRAY_FOREACH(ELEM, ARRAY)                                          \
        RD_ARRAY_FOREACH_INDEX(ELEM, ARRAY, __i)

[pranavrth]

There are places in librdkafka where we expect a variable to be defined already. We can expect __i defined already.

[emasab]

There are places in librdkafka where we expect a variable to be defined already. We can expect __i defined already.

I don't like it because you've to define the variable outside it and the name is fixed, in that case it's better if we leave only the original RD_ARRAY_FOREACH passing the variable name

[pranavrth]

How about using C99 standard of declaring variable inside the for loop. Like for(int i=0;i<n;i++) ?

[emasab]

That way it had problem with older versions of MSVC that didn't implement the C99 standard fully until some time ago.

[pranavrth]

Understood. I would prefer to remove this FOREACH loop altogether. But if you want to keep it, keep only RD_ARRAY_FOREACH_INDEX. Remove RD_ARRAY_FOREACH. So the implementation of original RD_ARRAY_FOREACH but with changed name RD_ARRAY_FOREACH_INDEX.

[pranavrth]

LGTM!.