Fix unused parameters in tscore

[freak82]

Add [[maybe_unused]] to unused parameters in the tscore functionality

[JosiahWI]

The AuTest http2_write_size_threshold failed.

[freak82]

The same error happened before and I was told that it's a transient error and not related to the changes.

[JosiahWI]

Yep, I rerolled the CI for you. It's a personal habit of mine to make a note of the failed test before rerolling, so that we can identify which tests are causing problems and improve them.

[freak82]

@cmcfarlen There was a discussion here for the preferred way, that's why I'm doing it with [[maybe_unused]]. I mean, I'm OK with any type as long as the final goal is achieved.

[cmcfarlen]

Commenting out the unused argument name is already a precedent in the ATS code and the compiler will not warn about it. There are also these defined for use in ink_defs.h:

#define ATS_UNUSED         __attribute__((unused))
#define ATS_WARN_IF_UNUSED __attribute__((warn_unused_result))
#define ATS_UNUSED_RETURN(x) \
  if (x) {}

Here is an example of how this is used:

int
LogConfig::reconfigure(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNUSED */, RecData /* data ATS_UNUSED */,
                       void * /* cookie ATS_UNUSED */)
{

In comments, this is expository, but here is another example:

  char ATS_UNUSED *ts_ms = int64_to_str(&tmp_buf[tmp_buf_size - 4], 4, ms, &num_chars_ms, 4, '0');
  ink_assert(ts_ms && num_chars_ms == 4);

which is also an example of when [[maybe unused]] would be appropriate; because ink_assert is conditional, some build configs will not use ts_ms

I think that we should reserve [[maybe unused]] for cases where the argument is sometimes used depending on circumstance (constexpr or other conditional compile). I do agree with the intent of these PRs to get the code to where we can enable the unused parameter warning.

@maskit What say you?

[maskit]

Some of the use may not be appropriate but I don't mind using [[maybe_unused]] for every place that compilers warn at this stage. This is just a step to enable the warning, and we should take another look more closely later. Some of the variables/arguments may be unnecessary at all. Then the caller side should be changed too. If we simply remove only the name, it'd be difficult to find parameter values that are unnecessarily passed.

Also, I don't like ATS_UNUSED macro. If some compiler supports __attribute__((specialized_unused_attribute)) and we want to automatically switch to the special one, having the macro would make sense, but we don't have such a thing. It only costs time to check the implementation for nothing.

[cmcfarlen]

The current method in the code is to comment the name and add ATS_UNUSED. This should be very easy to find later...

int
LogConfig::reconfigure(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNUSED */, RecData /* data ATS_UNUSED */,
                       void * /* cookie ATS_UNUSED */)
{

This occurs a lot in the code already. Shouldn't we stick to this?

[maskit]

The thing that would be hard to find is the caller side.

Let's say we have this function below. The second argument bar is obviously unused and it's not conditional.

int func(char *foo, char *bar) {
  return strlen(foo);
}

If we only look at the function definition, we may think we can change it like below

int func(char *foo, char *)

But then how do we find that this function call below passes the second parameter unnecessarily?

func("abc", generate_huge_data());

We should take a closer look and remove the second argument or stop calling the heavy function in this case if we can't remove the second argument.

What's the motivation to keep using ATS_UNUSED macro? It should be done in a constant way, but like I said, I don't see reasons to have the macro.

[cmcfarlen]

Just to be clear, I'm advocating for replacing int [[maybe unused] parm with int /* parm ATS_UNUSED */ in this PR to match the existing notation for unused parameters. I'm not advocating for using the ATS_UNUSED macro here.

[cmcfarlen]

Hello @freak82 , we discussed this at the review meeting and agreed that we should continue to use the /* parm ATS_UNUSED */ style for unused parameters. Could you update this PR to switch to that approach? Thank you for your help!

[freak82]

Hello @cmcfarlen, Thank you for the update. I'll update the pull request by the end of this week.

[freak82]

Updated this pull request with usage of /* name ATS_UNUSED */ is it was suggested. There is conditional compilation in the ink_memory.cpp and ink_queue.cpp and thus [[maybe_unused]] is used there.

[freak82]

Can't figure out why the Rocky build fails. Can somebody point me to the error because I can't find the reason.

[bneradt]

[approve ci rocky]

[JosiahWI]

It's the leak report from LSan. We are borrowing this to debug it.

[JosiahWI]

We're finished messing with CI on this PR now. The leak is still showing up (but we're leaving this on a passing CI so it's good for review and merge) and our idea to clear the ccache data in case it was related had no effect.

[cmcfarlen]

Cherry-picked to v10.0.x