Resolve conflict with tcl.h

[nijtmans]

Resolve possible conflict with tcl.h, allowing <tcl.h> and <tommath.h> to be #included in any order.

The problem is that Tcl has a few API's which are using the "mp_int" structure from tommath.h. For Tcl, the internal structure of mp_int is transparant, that's supposed to come from <tommat.h> But tcl.h contains the following code fragment now:

#ifndef MP_INT_DECLARED
#define MP_INT_DECLARED
typedef struct mp_int mp_int;
#endif

This allows Tcl to use mp_int in it's own API, without defining how it looks like: that's up to libtommath. The first header-file included - either tcl.h or tommath.h - will typedef "mp_int" as an anonymous struct, while tommath has full freedom to specify how it looks like.

So, I ask that tommath.h does the same: If another header file already typdefs mp_int as a anonymous struct, <tommath.h> doesn't need to do that again, just add the exact structure definition. This way the two header files can cooperate without conflicts.

At this moment, Tcl contains a modified <tommat.h> with this change. But it would be great if Tcl could include an unmodified <tommat.h> in the near future.

Thanks!
Jan Nijtmans

[minad]

No, such patches, which are only useful for Tcl won't happen. Please use a patched header in tcl.h. Please do not try to get this merged over and over again. I tried to point out a few times before that this is not a proper thing to do. Duplicating the struct definition in some other header. This can lead to very bad things, incompatible structs used in different compile units etc. Looked more closely, and you are just making the struct opaque, not redefining it. I still think it is the wrong thing to do, you cannot expect of third party libraries to be compatible with your style of predeclaring structs.

[minad]

strongly against this

[minad]

@nijtmans I wonder why do you not simply include tommath.h in tcl.h instead of predeclaring the mp_int struct? It seems mp_int is exposed throughout the tcl api at many places, but you seem only want to half-expose it through the predeclared struct. What is the reason for that again? I recall something like you wanted to link in msvc to ltm compiled by gcc and this way the struct type is invalid.

The other alternative would be to not expose tommath, by introducing some opaque TclBigInt struct. This would probably be preferable since then tcl is not tied to tommath for ever and the bigint implementation could be switched.

[nijtmans]

Looked more closely, and you are just making the struct opaque, not redefining it. I still think it is the wrong thing to do, you cannot expect of third party libraries to be compatible with your style of predeclaring structs.

No problem, we differ in opinion on this. You have the right to do that. In my opinion this is the best solution, but if I'm stepping over your limits, I will go for the second-best solution. No hard feelings. ;-)

[minad]

No problem, we differ in opinion on this. You have the right to do that. In my opinion this is the best solution, but if I'm stepping over your limits, I will go for the second-best solution. No hard feelings. ;-)

I thought a bit about this. This patch is relatively unproblematic and it wouldn't hurt us much to merge it. But I still believe that it is not the correct thing to do for many reasons. Sorry if I am repeating myself.

1. It is a hack. Interestingly not a hack needed here but by a downstream project to work around their own api quirks.

2. There should be something like a library boundary. It should be possible to argued that a change makes sense from the library point of view. However a library has to consider its users and use cases. So this criterion is pretty fuzzy and weak.

3. There is nothing wrong with having a simple struct definition as we are having right now. Some large libraries might provide predeclaration headers. This is very often a C++ thing to reduce compilation time. But we are talking about a single struct here. It would not hurt you to simply include the full header in Tcl instead of doing this predeclaration dance. (And if it would hurt you and you need to have the struct opaque due to some other reasons, it is a hack and we have go back to point 1).

4. I looked at your Tcl headers and how you are including or not including tommath.h. In contrast to all the other Tcl_types, the mp_int type stands out. It should either be fully embraced or wrapped to keep it internal. If it should be exposed, then it makes sense to simply include tommath.h everywhere to get access to the definition. If one wants to avoid exposing it (as I would argue), the mp_int type should not appear anywhere in the Tcl api. So what I am saying, the headers in Tcl could probably be reorganized a bit. (I only took a quick glance so ignore what I am saying if it is plain wrong. For example I couldn't figure out directly which headers belong to the public and which are meant to be private. Probably the tommath related headers are not user-facing for tcl embedders?)