-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Providing std::
functions, but not related headers can be problematic
#1
Comments
On additional datapoint (which is essentially a separate issue, but might need to be kept in account while fixing this one): In some cases, gcc's libstdc++ can provide functions from C++ versions newer than what It does seem to define the standard version for each such symbol (e.g. Note that I noticed this compiling outside of Arduino, but I would expect this to happen inside Arduino as well (given that at least the official cores tend to run with |
Hmm... these are difficult problems...
Yes, this is the reason why I haven't named I think the best way is a selective compile as you imagined, like excluding existing functions using If I have time (I can't say when is the estimated date...), maybe it can be achived to make this library works with How do you think about it? |
Yeah, I think this is the right approach. I'm already testing some things to maybe implement that, I'll get back to you (hopefully with a PR). I'll also tackle the Note that |
I tried some more fine-grained definitions, with some success, but I'm not sure what the best approach is yet. Below some of my findings and thoughts, partly for review, and partly to help me organize the subject matter and evaluate options :-) In my testing, I found some other complicating factors (mostly solvable or solved, just wanted to list them here):
Anyway, further analyzing this issue, I've found that there's basically three cases that we'd like to support:
What I tried now, is to define a To detect case 3, I've used I later realized that I could maybe also use I guess this will really stay a matter of detecting and handling specific cases (e.g. like ArduinoSTL), and adapting this handling when other libraries such as ArduinoSTL change. Really generically and portable defining this stuff is going to prove impossible, I'm afraid. Looking at this from the perspective of what is being defined, I think that:
So it seems that just defining a For the operator new, we can observe that, as shown above, all of I've pushed my work in progress for review, see PR at #2. I also realized there is completely separate and possibly simpler way to fix all this: Instead of defining stuff in the On platforms where This approach would make some sense, since any code that uses If a sketch or other code is sure that it does not include any How would you feel about something like this? This would of course require a one-off change in all users of the library... |
I found one more problem wrt to my |
I almost agree with your proposition. Thanks a lot :)
It's not a best way, but I'll add
A part of C++17 features are included only on openFrameworks in Windows... like
Agree :)
I wondered which is better to use I also got one more problem about I have tested your current PR on some libraries which use ArxTypeTraits, so I'll add some comments to your PR. Thanks again! |
Ah, I see, makes sense then. There's still one caveat that in this case the guards only work if OF is included before ArxTypeTraits (unlike the libstdc++ void_t, since the ArxTypeTraits itself includes
Yeah, I have this feeling as well. That would make most of my PR irrelevant, though. I just realized that this approach would also make the conflicts with OF and I gave the I wonder one more thing, though: even when switching to Another approach is to use stuff from Let me know what you think, after lunch I'll probably try merging both approaches into a single PR. |
Thanks! I prefer to use existing items first. The priority is standard library > ArduinoSTL > ArxTypeTraits. ArxTypeTraits is just supplements for the missing part of them, I think :) |
One caveat with A possible workaround could have been to not use For this same reason, things like Doing this will lose support for ArduinoSTL on non-AVR cores (since saying "if is available, you'll probably have ArduinoSTL and only support C++98" will certainly be false on architectures that have libstdc++). However, there is the This might actually allow more generic detection: Just see if some C++ header is available, and if it's libstdc++ or libc++ (just for completeness), assume that |
I just updated the PR with the above refactoring. I think I'm pretty happy with the resulting code as it is now. The commit history is a bit of a mess still, I can try to clean that up if you agree that the current approach is ok? One more possible improvement I thought of would be to put the There is the question of compatibility now, this change will need all users to update their code. In particular with the bundling of libraries you do, this might become somewhat tricky (i.e. if you update ArxTypeTraits but not MsgPack, or the other way around, things will end up breaking). More generally, whenever two copies of your libraries are included in the same project (e.g. bundled in two libraries, or one directly and one bundled) you will not get duplicate definitions because the include guards for these files prevent only one copy to be loaded, but if the versions of both copies differ, you might end up with broken code (when a user assumes they get version x, but they end up with an older or newer but incompatible version). For this, it might make sense to let e.g. |
See also hideakitai/MsgPack#5 for the related MsgPack and ArxContainer changes. |
Thank you so much! I really appreciate it.
I agree. I want to do that after checking if it works but before merge PR.
For me, I will change all of my libraries after merging PR.
Agree. I knew I had to do it, but now it's the time...! I think it is ok not to allow newer version (compilable only when the versions are exactly same). Then, summarize the rest of tasks... |
The rest of tasks are like this?
|
I've checked the ArxTypeTraits #2 and hideakitai/MsgPack#5.
So how about using just It enables to switched between libstdc++ (or ArduinoSTL) and ArxTypeTraits by switching |
Yup, looks good,.
With the current PR, if stdlibc++ is available, it is included and its stuff is exposed under
Yeah, as I said before, trying to reference things from
I would not do this, because it muddies the water between the "std::" stuff implemented or imported by ArxTypeTraits, and the stuff that it adds to it. Putting everything in
I think you can now do this already with
Note sure what you mean here? |
Ah, I see what you mean there: If you do Lines 35 to 39 in dbe4018
But having to modify all references to Also, my previous suggestion of adding So if we want the arx-std-stuff in its own namespace, but it cannot be |
I read codes and comments above again and tested, sorry, my understanding was completely wrong (and sorry for my poor reading/writing of English...). About 1, yes, this is automatically realized by using Lines 35 to 39 in dbe4018
About 2, this means exactly what you said in the comment above. About 2 and For example, use only |
I just tried adding this to ArxTypeTraits directly:
(note that I also renamed As suggested before, this makes sure that everything that is defined by arx is again imported back into the As for the
How would that help? You'd still have
What are you saying here? That |
One thing I do not know is how this would work for e.g. adding specializations or extra overloads, but I suspect that's not currently done in ArxTypeTraits, it just adds names that were (maybe) not defined by the standard library. |
I pushed updates to both PRs just now with the above changes. Because everything is available through |
Yeah, renaming to I previously proposed to use
You mean this is same as above? Of course current
I just want pretty alias for |
I've checked and worked perfectly... super cool. Thanks so much! :) |
Ok, #2 is finished, I think. Looking at the other things on your list (updating other libraries etc.), I think it would make most sense if you did those, also because I'm not really using anything else than MsgPack currently. I'd be happy to review stuff, though. |
Thank you so much for your awesome contribution! Close this issue with merging #2. :) |
I've been using this library today (rather, the one bundled with MsgPack, but that's the same as far as this issue is concerned) and ran into some issues.
In my sketch, I'm using the
ArduinoSTL
(uclibc++ and STL) library, which contains implementation of most of C++98 stdlibc++ (which is otherwise missing on AVR), making everything available under the standard names (e.g.<algorithm>
). When I add theMsgPack.h
include, this starts producing conflicts becauseArxTypeTraits
defines some classes and functions, which are also defined by ArduinoSTL. e.g.:I already tried two things to work around this, without luck so far:
#include <algorithm>
from my code and rely on ArxTypeTraits to provide the needed declarations and implementations. However, this makes the code less portable, and more importantly does not work when using third party code (e.g. I'm also using ArduinoJson now, which also does#include <string>
(which indirectly includes<algorithm>
as well).#define ARX_TYPE_TRAITS_DISABLED
) and instead letting it include standard header files (which would then include ArduinoSTL), but there I ran into the problem thatuclibc++
does not offer<tuple>
, since that was added in C++11.One way to fix this seems to be to let ArxTypeTraits offer the functionality under the proper header names, e.g. let it actually have a file called
algorithm
. This does have some downsides:<tuple>
, this would work because ArduinoSTL does not offer the file at all, so the include would fall through to ArxTypeTraits, but for other stuff, likestd::enable_if_t
, that was added to existing header files, this would not work.util
subdirectory and not on the include path (but given it is a header only library and uses#ifndef
header guards, it could maybe work in this case to just include the library twice: Once bundled in e.g. MsgPack, and once as a standalone library, as long as the libraries define the same things, that should be ok).Another way I can imagine might work, is to tell
ArxTypeTraits
about what parts of libstdc++ are (not) available. Right now, it assumes that on a few Arduino architectures, libstdc++ is completely unavailable, and defines parts of it itself, and everywhere else it assumes that libstdc++ (C++11 always, newer versions depending on the__cplusplus
macro). I'm ignoring the handling ofnew.h
now, which I think might need fixing separately.It could help if we could just use ArduinoSTL and then tell ArxTypeTraits (e.g. through a macro or so) that libstdc++98 is available, letting it just include the standard header files, and defining only the things that were added in c++11 and later. This might still end up being somewhat problematic (since I think ArduinoSTL currently defines some things from c++11 but far from everything), but it might be a good place to start.
For now, this is not blocking me since I can test on other platforms than AVR for a while, but I might end up trying the latter approach at some point.
The text was updated successfully, but these errors were encountered: