-
Notifications
You must be signed in to change notification settings - Fork 24
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
Cleaner namespace #189
Cleaner namespace #189
Conversation
This primarily consists of removing platform.h from the public namespace. It is still included in federate.h, but this is not a problem that needs to be dealt with right now.
There might have been a reason why this was created, but I consider it to be excess generality. I do not see us moving toward representing time with less than 64 bits in the near future unless I missed something in our conversations on the topic, and I question whether emulation of 64 bit integers is as much slower than the alternatives as it is imagined to be. It has also been commented multiple times that defining _instant_t in one place and then always aliasing it as instant_t smells like overengineering.
Yes, windows.h really is that broken. The mess that comes with Windows support in C is the reason for this namespacing struggle over the past couple days and it is abominable and in principle avoidable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! (modulo the org.lflang.util.TargetResourceNotFoundException
in https://github.com/lf-lang/lingua-franca/actions/runs/4569612822/jobs/8066054860?pr=1599)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a nice cleanup. However, how many tests and examples have to be changed? The corresponding PR #1599 in lingua-franca seems to bundle this with bodiless reactions and has 263 files changed (yikes!!!), so this does not look like something even remotely backward compatible. Everything in the examples repo, the website examples, and various papers will have to be changed as well. Also, some of these changes look far from trivial. Why does Deadline.lf require the following:
preamble {=
#ifdef __cplusplus
extern "C" {
#endif
#include <stdio.h>
#include "platform.h"
#ifdef __cplusplus
}
#endif
=}
Are we expecting end-users to put this into their code?
I suggest factoring out from the lingua-franca the part of the PR that pertains only to this namespace, decoupling it from the bodiless reactions. We could then get a much clearer sense of what the implications are.
Also, then we could get tests passing before this is merged.
Also, I think that while it is currently true that all platforms use int64_t for instant_t, the original design was intended to make it possible to use something different. This is why the intermediate types _instant_t, etc., were defined. However, we backed away from the with Arduino, and to date, nobody has come up with a reasonable design that can use smaller time representations. So I think it is OK to remove this speculative design and leave it whoever realizes a different time representation to come up with a design.
However, to make this possible future change backward compatible, maybe instant_t, interval_t, and microstep_t should be defined in platform.h, not in tag.h. That way, any user code that uses these types, e.g. outside a .lf file, will have to #include
platform.h, and any future design can have platform-dependent definitions.
No, certainly not! I assume that
Furthermore, the reason for the preprocessor stuff (
We can do that, but I claim that it would accomplish very little because I think that all of the non-backwards-compatible changes pertain to the cleaning up of the namespace. The tests got changed because they were depending on functionality that shouldn't necessarily be exposed to end-users; now, they have to explicitly I would like to emphasize that the vast majority of the work that is involved in this PR and the
Sure, no problem! |
The tests probably failed because I forgot to update |
In this issue #161 we discuss what should be the user-facing API. And at least we conclude that we should expose Another way to make the namespace cleaner is to have all reactor specific header files prefixed by: reactor-c. e.g. "reactor-c/platform.h". I think this makes sense because platform.h etc. is a very common name of a headerfile |
I believe that this is reasonable. But what is not a clear advantage is exposing these APIs automatically, whether the user
I am aware that in many of our examples we spawn a separate thread that does polling and schedules physical actions. But I am not sure I understand why this makes sense. If you do the polling in the operating system instead of in LF, then this does not magically create an extra processor that does this polling on the side. The operating system has to do context switches in order to give cycles to this asynchronous polling thread from time to time. AFAIK the reason why this idiom makes sense in most sequential programming languages is that in most sequential programming models it is inconvenient to program (a separate parallel task that periodically gets cycles); however, it is very convenient to program in LF. But this is a side discussion. The point is that, OK, maybe we do want this to be available to the user (although I am still not sure), but that doesn't mean we should put it in their namespace without their asking for it.
I (strongly) agree with this, and it does have something to do with namespacing, but I don't see how it's relevant here. The problem that I am trying to solve in this and the other PR is that the user-visible code |
One reason our programs create threads is to test physical actions. How else would you test them? A second reason is to provide user input that is not polled. If you want to read from the keyboard, for example, the most portable mechanism in C is a blocking call like |
OK, like I said, I have no problem with this, but it is a side discussion. I concede that the user may in some cases need threads, but I am not sure that we need to put them in the namespace of all LF programs automatically whether the user wants them or not. It is not a crazy or radical idea that programmers should have control over what functionality they import, especially in a language like C where there is no language support for namespace qualifiers. |
I think that in the long run this might not be what we want, but it reduces the impact of this and the accompanying lingua-franca PR on existing LF programs.
93e65c5
to
9eb95b0
Compare
I agree. I think explicit imports are a good idea. I was just trying to explain the use cases for threads. |
This is because although time types are not platform-dependent currently, they could be in the future.
Actually, I think it is slightly problematic to keep the typedefs for time types in Edit: I should clarify that I really do not want |
This reverts commit d008e46. This is causing build problems that would be hard to fix without bringing platform.h into user namespace. See #189 (comment)
My logic is based on these two assumptions:
If we accept these two, and there are other platform-dependent mechanisms that we do not want in the user namespace, then I see no choice but to have the platform logic of platform.h replicated in at least two different files, one that is in the user namespace, and one that is not. This is not a disaster, but it is unfortunate because that logic is complicated, and adding a new platform will require doing so in two or more places. What are the definitions in platform.h that you feel should not be in the user namespace? The ones related to threads? Any others? One way to resolve this issue is to think of it from the perspective of documentation. Currently, we have no clear documentation of the definitions that are available to a user, neither the ones that are automatically in their namespace, nor the ones available only through #include of files that are automatically made available to them, nor the ones that have to also be included using the files and cmake-include target properties. Is suggests that this PR should be accompanied by such docs. Only then can we evaluate the choices. |
Yes, I agree, this becomes a non-issue when we have two different files.
For me, the most important thing is that we make sure that platform dependent things such as Windows headers, pthreads primitives, and the like, which may (accidentally?) have leaked into
Okay, I will take it as an action item this week to document which definitions are available and to what extent, as you describe. |
For the docs, it may be as simple as three bullets that point to auto-generated docs:
|
I was able to generate the docs locally, but since they do not seem to distinguish between declarations that come from the different header files they did not seem to be of much help here. Unfortunately I do not have a lot of time right now for making the docs look nice, but this is what I had in mind when I did this refactoring: Declarations available without
|
@edwardalee, this PR is still pending your approval after you requested changes. I think that @petervdonovan has addressed all the points you brought up, so I think it's good to go. |
Yes, this is good to go, as far as I'm concerned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Peter may have been right to remove <stdio.h> from the automatic include, so if you want to go back to that, I'm ok with it. But it is painful as most programs out there will fail to compile... Your call.
He already removed that change. If we want to remove stdio from the namespace, we can do it in a separate PR. But I don't see that as a priority (esp. considering the headache of updating existing code). |
See lf-lang/lf-lang.github.io#135. I'm not sure about the level of detail that we want, especially since this might continue to change a little bit as we make tweaks to the build system. If we want to list a complete API doc, then I can do that, but it would take a long time. |
Reduce the namespace pollution that leaks into user-level code. This was originally motivated from a collision with something in the depths of the
<windows.h>
mess.The accompanying
lingua-franca
PR is lf-lang/lingua-franca#1599.