Skip to content
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

Merged
merged 12 commits into from
Apr 12, 2023
Merged

Cleaner namespace #189

merged 12 commits into from
Apr 12, 2023

Conversation

petervdonovan
Copy link
Contributor

@petervdonovan petervdonovan commented Mar 30, 2023

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.

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.
Copy link
Member

@lhstrh lhstrh left a 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)

Copy link
Contributor

@edwardalee edwardalee left a 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.

@petervdonovan
Copy link
Contributor Author

Are we expecting end-users to put this into their code?

No, certainly not! I assume that platform.h is for our internal use, and that most of the time we do not intend for end users to #include it. In particular, I think it is bad for end-users to #include it because

  • if they do that on Windows, then they see <windows.h>, which is a big mass of namespace pollution, and
  • they might have access to other things that they should not be using, such as concurrency primitives and sleep functions (if users want those, then they should not use LF).

Furthermore, the reason for the preprocessor stuff (#ifdef __cplusplus etc.) is that we try to compile the same code both as C and C++, which end users should never do.

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.

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 #include all of that functionality in order to work.

I would like to emphasize that the vast majority of the work that is involved in this PR and the lingua-franca PR pertains only to the namespace.

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.

Sure, no problem!

@petervdonovan
Copy link
Contributor Author

Also, then we could get tests passing before this is merged.

The tests probably failed because I forgot to update lingua-franca-ref.txt. They were passing in the main lingua-franca repo. Let's see if they pass now.

@erlingrj
Copy link
Collaborator

In this issue #161 we discuss what should be the user-facing API. And at least we conclude that we should expose lf_thread_create and lf_sleep. Maybe he mutexes and condition variables also?

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

@petervdonovan
Copy link
Contributor Author

petervdonovan commented Mar 31, 2023

If we already provide a platform, then why should hide it from users? More use of our platform APIs will lead to more portable code... That seems to be a clear advantage to me. Is there a cost to this that I'm not appreciating?

I believe that this is reasonable. But what is not a clear advantage is exposing these APIs automatically, whether the user #included them or not.

at least we conclude that we should expose lf_thread_create and lf_sleep

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.

Another way to make the namespace cleaner is to have all reactor specific header files prefixed by: reactor-c.

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 #includes reactor.h for access to the API, which then includes something else, which includes something else, which includes the whole universe, all when the user only needed the core LF API. This will happen regardless of the path that is specified when we include platform.h.

@edwardalee
Copy link
Contributor

If we already provide a platform, then why should hide it from users? More use of our platform APIs will lead to more portable code... That seems to be a clear advantage to me. Is there a cost to this that I'm not appreciating?

I believe that this is reasonable. But what is not a clear advantage is exposing these APIs automatically, whether the user #included them or not.

at least we conclude that we should expose lf_thread_create and lf_sleep

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.

Another way to make the namespace cleaner is to have all reactor specific header files prefixed by: reactor-c.

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 #includes reactor.h for access to the API, which then includes something else, which includes something else, which includes the whole universe, all when the user only needed the core LF API. This will happen regardless of the path that is specified when we include platform.h.

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 getchars(). You don't want to call a blocking function in a reaction. In an embedded platform, the equivalent is an interrupt service routine. You can think of that as a thread that blocks on interrupt requests.

@petervdonovan
Copy link
Contributor Author

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.
@edwardalee
Copy link
Contributor

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 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.
@petervdonovan
Copy link
Contributor Author

petervdonovan commented Mar 31, 2023

Actually, I think it is slightly problematic to keep the typedefs for time types in platform.h because then that forces us to #include "platform.h" in tag.h, which means that platform.h is in scope in the user namespace. There are various ways that we could fix this, but I think they would result in more refactoring than I would prefer to do right now.

Edit: I should clarify that I really do not want platform.h in the user namespace by default because of windows.h. We can fix that, too, but it would also be a bit of work and it would make this change set even bigger than it already is.

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)
@edwardalee
Copy link
Contributor

My logic is based on these two assumptions:

  1. Since time is central to LF, instant_t should be visible to a reaction without any special #include. So should the lf_time... functions.

  2. The definition of instant_t is potentially platform dependent, as are lf_time...

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.

@petervdonovan
Copy link
Contributor Author

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.

Yes, I agree, this becomes a non-issue when we have two different files.

What are the definitions in platform.h that you feel should not be in the user namespace? The ones related to threads? Any others?

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 platform.h, are not in the user's namespace. If platform-independent functions like lf_thread_create and lf_nanosleep also appear in the user namespace, then I will not be particularly pleased, but I think it would be fine.

Is suggests that this PR should be accompanied by such docs. Only then can we evaluate the choices.

Okay, I will take it as an action item this week to document which definitions are available and to what extent, as you describe.

@edwardalee
Copy link
Contributor

For the docs, it may be as simple as three bullets that point to auto-generated docs:

  • Definitions available immediately in a reaction body (how to #include them in external files?)
  • Definitions that require an explicit #include
  • Definitions that require a #include, a files entry, and a cmake-include (this should be a sublist and should point to the util files).

@petervdonovan
Copy link
Contributor Author

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 #include

  • Functions and macros used to set ports and iterate over multiports
  • Functions and macros used to schedule actions
  • Functions and macros used to set a reactor's mode
  • Functions and macros used to create trace points
  • Logging utility functions
  • Typedefs relating to time and logical time, including tag_t, instant_t, interval_t, and microstep_t
  • API functions for obtaining timing information about the current program execution, including the current physical and logical time

It is possible that other generally useful header files may be exposed to the user, such as stddef.h and stdio.h, for example. However, users who rely on the inclusion of these header files in reactor.h should expect breaking changes between releases.

Declarations that require an explicit #include

  • platform.h, for the purpose of accessing platform-independent APIs that are based on platform-dependent implementations.

It is possible to include other functionality that is also part of the runtime. However, this is not recommended, and users who do that should expect breaking changes between releases.

Declarations that require a #include, a files entry, and a cmake-include

  • Anything in the utils directory.

@lhstrh
Copy link
Member

lhstrh commented Apr 12, 2023

@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.

@edwardalee
Copy link
Contributor

Yes, this is good to go, as far as I'm concerned.
We need to get the docs Peter drafted onto the website, but I can take that on.

Copy link
Contributor

@edwardalee edwardalee left a 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.

@lhstrh
Copy link
Member

lhstrh commented Apr 12, 2023

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).

@lhstrh lhstrh merged commit 667a5d7 into main Apr 12, 2023
@petervdonovan
Copy link
Contributor Author

We need to get the docs Peter drafted onto the website, but I can take that on.

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.

@petervdonovan petervdonovan deleted the cleaner-namespace branch April 12, 2023 21:09
@lhstrh lhstrh added the enhancement Enhancement of existing feature label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants