Make json serialisation methods static inline

[mutagene]

Previous switch from nlohmann-namespaced adl_serializer(...) method to putting the from_json/to_json in the same namespace as the json types can fall afoul of "Unused function from_json" warnings/errors when compiling with flags -Wunused-function or -Wall.

This change makes the definitions static inline to avoid such errors as it is quite common if e.g. logging to only ever use the to_json methods.

[jothepro]

I understand that the warnings by the compiler are not necessarily helpful in this case since the functions are generated and cannot be eliminated individually just because some of theme are not used.

But I have a few questions to help me better understand the provided solution:

• Did you test if this works with all major compilers? It seems like this may only hide the error in gcc but not in clang?

  CC explicitly does not warn for unused static inline functions for -Wunused-function. [...] Clang does warn for static inline functions that are unused.

  (Source)

• From my understanding the compiler is not wrong when he is complaining that the function is unused. Did you consider to explicitly disable the warning with the [[maybe_unused]] attribute instead of implicitly hiding it with the inline keyword?
• I am trying to imagine if there is a situation where one is actually interested in these warnings and it would be a problem if they don't occur. Do you think that's a valid scenario as well?

Bear with me if my questions indicate that I may not have fully grasped the issue yet. The inline keyword is new to me and I had to do some research first, which lead me to the above questions.

[a4z]

This is a generated header, from some point of view like a system header. As such, it should not cause any warnings in user code.

This is usually archived by including such headers from a directory specified as -isystem. Then there will be no warnings. But the user needs to take care of that, specifying -isystem correctly.

From that point of view, we have nothing to do.

On the other hand, I think having an inline on functions that are implemented in a header that is not a template is correct. Otherwise, we could end up with multiple implementations of the same function if the header is included more than once. And therefore should be done. But I do not think we need the static keyword in this context.

On the other hand, for the template classes, we should not need the inline since templates should be inline per definition.

So yes, I think some things can be done with that code to make it look better, and this PR works in the right direction.

Can we give it a try, inline for free functions and no inline for templates? And if there are warnings, our tests should cover them ... in an optimal world. This needs of course not be done in the context of this PR, because I do not want to add a lot of work to @mutagene . So maybe a follow-up ticket could be nice ...

[a4z]

Hi @mutagene , I would like to get that change in But do you have the possibility to check if that would work, no static and just inline for the free functions and no inline for template functions?

Would be awesome if you could, since you also seem to have code that triggers problems, in case there are some.

[mutagene]

Sorry for the delay. I'm really grateful for your thoughtful feedback and I'm sorry I haven't had time to respond properly. I'll take a look soon.

[a4z]

Hi @mutagene , we recently added some fix to the generator. That speaks for a new release.

Do you think this PR has a chance to receive some updates the next time? Because if it does, we could well wait to have this improvement onboard.

[mutagene]

Thank you!

I added some suggestions since I think the static inline is redundant, and the static can be removed.

Klick through them if you want or leave it; we will take that in the future. It's more of a cosmetic fix.

Yes, I completely agree.

[a4z]

@jothepro , can you please glance over the PR and merge it if you think it's OK? (I would appreciate a second pair of 👀 since I added changes myself)

[jothepro]

I plan to do a full review tomorrow. 👍