Skip to content

Conversation

@SolidWallOfCode
Copy link
Member

@SolidWallOfCode SolidWallOfCode commented Aug 21, 2018

This enables conditional compilation checks in pure C++ code, without support from configure.ac or #define. It is a bit more verbose but has the advantage of being self contained - the code does not depend on external tools in order to compile correctly. It is also more robust because it does its checks with the exact compiler and library environment. Therefore changing the environment and recompiling will change the output without other rebuilding.

The basic structure is a wrapper function is created to cover the conditional code. A set of "case" functions, analogous to switch / case, are created as overloads. These are called from the wrapper, and checked by the compiler in order and the first one that successfully compiles is chosen. The checking involves attempting to compile an expression - if the expression compiles successfully that is a success. It is not a visible error if the expression does not compile, except for the base case because at least one of the overloads must be a success. It is not an error if multiple overloads succeed, the highest ranked will be used. This is a critical difference from std::enable_if, which requires disjoint success that can be very challenging to achieve.

An example of its use in a real life situation is here in #4040. This creates a wrapper function for an openSSL call that compiles to the underlying call if present, and nothing if not. In that case, the conditional compilation check is

SSL_CTX_set1_groups_list(static_cast<T *>(nullptr), static_cast<char*>(nullptr))

This compiles if there is an SSL_CTX_set1_groups_list function that takes a T (in this case an SSL_CTX because the wrapper function forces that) and a char *. If not, that case is not used. Note that if BoringSSL has something similar but with a different name, another case could be added (with CaseArg_2) to check for that and provide a shim to call it correctly.

@SolidWallOfCode SolidWallOfCode added this to the 9.0.0 milestone Aug 21, 2018
@SolidWallOfCode SolidWallOfCode self-assigned this Aug 21, 2018
@masaori335
Copy link
Contributor

Looks reasonable and I prefer this style than detecting APIs by m4 macro.

@maskit
Copy link
Member

maskit commented Aug 22, 2018

The idea is interesting and I'm not going to stop this for now, however, I'm not so sure this can be used widely so far.

I understand OpenSSL vs BoringSSL is just an example and their old APIs can be wrapped with this, but I don't think it can cover all cases. For example, some cases may need extra function calls at other places and usage may completely different (sync/async), which means we would need ifdefs somewhere anyway.

If above was true, it would make inconsistency like "To do A, you can use meta (TS_SSL_x), but to do B, you need ifdef SSL_y_init and SSL_y_fin". This doesn't sound great.

Another concern is that it's not feature oriented. Let's say we need 3 APIs to achieve a feature. What will happen if a platform / library supports only 2 of them? In this case, we need to check the all availability and define one flag to indicate that all the APIs are available and we can achieve the feature.

It would be great if you could show me more examples. Especially, examples that make existing code great (but not OpenSSL vs BoringSSL).

@SolidWallOfCode
Copy link
Member Author

@maskit - I think it's a rather unreasonable requirement that any new utility be able to handle every possible case. My view is every reduction of using the C preprocessor is a good thing and therefore a utility that enables that in many cases is valuable, even if it doesn't cover all such uses.

As for the inconsistency, that's only a style issue. You could just as easily wrap the #ifdef code in a wrapper function. E.g. in this case use #ifdef to define the contents of TS_SSL_CTX_set1_group_list. I've always preferred that style, because it moves implementation details out of the way of the main code path, as with ink_strlcpy.

Handling the multiple feature case turns out to be easy in most cases as well. Because of the abuse of the comma operator in the decltype, there's no limit on how many expressions can be checked in one case. If three are needed, all three can be checked in the same case.

This utility originally came from work I am doing on upgrading the IP address support code in ink_inet.h. I wanted to get rid of the ugly code about setting the socket length. I did a version of this as a one off to make a set_sockaddr_len function. Afterwards I realized I could use this in other similar circumstances and made this more generic form of the code. I'm still exploring use cases and I thought the particular one under discussion was a nice one.

@maskit
Copy link
Member

maskit commented Aug 22, 2018

@SolidWallOfCode

My view is every reduction of using the C preprocessor is a good thing and therefore a utility that enables that in many cases is valuable, even if it doesn't cover all such uses.

Partially agreed. It doesn't have to cover all use cases. I wanted to point out the inconsistency issue.

As for the inconsistency, that's only a style issue. You could just as easily wrap the #ifdef code in a wrapper function.

I was talking about a case that we cannot easily provide a wrapper. When I tried to support BoringSSL on QUIC code, I found a case that we cannot simply replace OpenSSL function calls. I haven't figured out the case, but I would probably need to call BoringSSL functions from different places and the logics for each libraries would be pretty different. I don't think this case has to be covered by this utility but it suggests that making TS_SSL_x doesn't work every cases, and I think ifdef is suitable in this case.

So, this would lead the same issue as ink_strlcmp. One reason I don't like making this kind of wrappers on lower layer is that I need to check whether wrapper X for function x is provided before using function x. This is a reason I didn't create ink_be64toh (See ink_endian.h).

I can live with it but I wanted to know how many places (and how much) we can improve with this.

Handling the multiple feature case turns out to be easy in most cases as well. Because of the abuse of the comma operator in the decltype, there's no limit on how many expressions can be checked in one case. If three are needed, all three can be checked in the same case.

Alright, sounds like it's possible. But again, I want to see how much it makes code readable and beautiful. Can you make separate PRs that use this utility so that I can see the improvements?

@SolidWallOfCode
Copy link
Member Author

Not in the near future. This is here because Masaori asked to see it. I'm using it in another project, as noted, having to do with refreshing the IP address support, but that won't be ready for a while. I suppose I could separate out the use of this as a PR, it's relatively independent.

@dragon512
Copy link
Contributor

I get what you are doing... I think we can clean up the code to make it more obvious. ( this was not completely obvious... there are limits :-) ) I will try to see what I can add to this

@SolidWallOfCode
Copy link
Member Author

I used this in several places in the Canned-YAML work, sometimes new but at least once to replace
hand rolled code that did the same thing.

@SolidWallOfCode SolidWallOfCode force-pushed the ts-meta-case branch 2 times, most recently from 7feb3d1 to 562e388 Compare October 11, 2018 02:44
@dragon512 dragon512 merged commit 59ed675 into apache:master Oct 12, 2018
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.

4 participants