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

Update SET, schedule and tag APIs in the C target #1103

Merged
merged 14 commits into from
Apr 25, 2022

Conversation

housengw
Copy link
Contributor

@housengw housengw commented Apr 19, 2022

This PR changes the SET, schedule and tag APIs in the C target. Here are the changes:

Format:

  1. old name -> new name means that the API is renamed from old name to new name. old name is still available in the current release, but will be removed in a future release.
  2. old name (deprecated) means that the old name has not been renamed, and will be removed in a future release.
  3. new name (new) means that the API is introduced in the linked PR.

C target:

  • SET related APIs:
    • SET -> lf_set
    • SET_ARRAY (deprecated)
    • SET_NEW (deprecated)
    • SET_NEW_ARRAY (deprecated)
    • SET_PRESENT (deprecated)
    • SET_TOKEN -> lf_set_token
    • SET_MODE -> lf_set_mode
    • lf_set_destructor (new)
    • lf_set_copy_constructor (new)
  • schedule related APIs:
    • schedule -> lf_schedule
    • schedule_int -> lf_schedule_int
    • schedule_token -> lf_schedule_token
    • schedule_copy -> lf_schedule_copy
    • schedule_value -> lf_schedule_value
    • check_deadline -> lf_check_deadline
  • tag_t related APIs:
    • compare_tags -> lf_tag_compare
    • get_elapsed_logical_time -> lf_time_logical_elapsed
    • get_logical_time -> lf_time_logical
    • get_current_tag -> lf_tag
    • get_microstep (deprecated)
    • get_physical_time -> lf_time_physical
    • get_elapsed_physical_time -> lf_time_physical_elapsed
    • set_physical_clock_offset -> lf_set_physical_clock_offset
    • get_start_time -> lf_time_start

These are copied from #1108. Please refer to that issue for details

@housengw housengw changed the title Undefine ctarget macros at the end of the generated .c file Change SET APIs to lowercase and deprecate uppercase variants Apr 19, 2022
@cmnrd
Copy link
Collaborator

cmnrd commented Apr 19, 2022

I don't know the background of this change, but I would like to comment on another aspect of naming in C. "set" is a very common term and likely used in many C libraries. By using such a common function name (SET or set) we are at risk of getting name conflicts with external libraries. Since there is an API change anyway, I think it is worth considering using a prefixed form lf_set to make name conflicts less likely in C.

@edwardalee
Copy link
Collaborator

Undefining the macros will have the side effect that set cannot be invoked in any function defined in a preamble. I think this is OK, but we need to understand that this is a restriction. If we go with a more proprietary naming convention, as @cmnrd suggests, like lf_set, then it become less imperative to undefined the macros. Let's discuss.

@lhstrh
Copy link
Member

lhstrh commented Apr 19, 2022

Undefining the macros will have the side effect that set cannot be invoked in any function defined in a preamble. I think this is OK, but we need to understand that this is a restriction. If we go with a more proprietary naming convention, as @cmnrd suggests, like lf_set, then it become less imperative to undefined the macros. Let's discuss.

Not being able to invoke set in a preamble sounds like a great feature to me, actually.

@Soroosh129
Copy link
Contributor

Another way to avoid this conflict once and for all is to put set and schedule (as pointers to functions) inside a global variable, such as ctx (inspired by the Rust target) or lf, or even self.

The programmer would then access them via ctx->set, etc.

@housengw
Copy link
Contributor Author

housengw commented Apr 19, 2022

lf->set is less preferable in C than lf_set IMO, and the fact that SET is a macro might make it difficult to put inside a struct.

@Soroosh129
Copy link
Contributor

Soroosh129 commented Apr 19, 2022

Not being able to invoke set in a preamble sounds like a great feature to me, actually.

I agree that this is a feature.
We want to prevent asynchronous calls to set in the preamble, and I think it's just safer to disallow set outside reaction bodies altogether. However, no one can stop a programmer from passing a pointer to set to an asynchronous thread (or just an ordinary function defined in the preamble).

@Soroosh129
Copy link
Contributor

[T]he fact that SET is a macro might make it difficult to put inside a struct.

You're right.

@housengw housengw self-assigned this Apr 19, 2022
@housengw housengw marked this pull request as draft April 19, 2022 23:17
@housengw housengw added the c Related to C target label Apr 20, 2022
@housengw housengw changed the title Change SET APIs to lowercase and deprecate uppercase variants Update SET, schedule and tag APIs in the C target Apr 21, 2022
@housengw housengw marked this pull request as ready for review April 22, 2022 21:23
@housengw housengw merged commit ca2689a into set-destructor-variant Apr 25, 2022
@housengw housengw deleted the deprecate-uppercase-set branch May 2, 2022 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c Related to C target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants