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

Make SET work with both token and non-token types #66

Merged
merged 44 commits into from
Apr 26, 2022
Merged

Conversation

housengw
Copy link
Contributor

@housengw housengw commented Apr 12, 2022

Added a destructor variant of SET and changed the signature of SET_ARRAY.
Refer to #65 for details.

The LF tests related to this change are added here: lf-lang/lingua-franca#1097

This PR makes SET work with both token type and non-token types.
This PR also includes changes from #68 since that PR was merged into this PR before merging into main.

@housengw housengw added the enhancement Enhancement of existing feature label Apr 12, 2022
@housengw housengw self-assigned this Apr 12, 2022
@housengw housengw linked an issue Apr 12, 2022 that may be closed by this pull request
4 tasks
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.

Let's also change the names to lower case, as suggested, leaving the old names, marked deprecated, for backward compatibility.

core/reactor.h Outdated Show resolved Hide resolved
core/reactor.h Show resolved Hide resolved
core/reactor.h Outdated Show resolved Hide resolved
core/reactor.h Outdated Show resolved Hide resolved
core/reactor.h Outdated Show resolved Hide resolved
core/reactor.h Show resolved Hide resolved
core/reactor.h Outdated Show resolved Hide resolved
include/ctarget.h Outdated Show resolved Hide resolved
@housengw
Copy link
Contributor Author

From the error message it seems like the lower case set macro is clashing with another function somewhere, probably in included libraries.

/SDKs/MacOSX12.1.sdk/usr/include/c++/v1/istream:163:
    In file included from /Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/c++/v1/ostream:141:
    /Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/c++/v1/bitset:703:17: error: too few arguments provided to function-like macro invocation
        bitset& set() _NOEXCEPT;
                    ^
    /Users/runner/work/reactor-c/reactor-c/test/C/src-gen/target/HelloWorldCCPP/ctarget.h:69:9: note: macro 'set' defined here
    #define set(out, val) _LF_SET(out, val)

@housengw housengw changed the title Add destructor variant of SET make SET work with both token and non-token types Apr 18, 2022
@housengw housengw changed the title make SET work with both token and non-token types Make SET work with both token and non-token types Apr 18, 2022
Copy link
Contributor

@Soroosh129 Soroosh129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for cleaning up the C API.

Superficially, should files like ctarget_schedule.h be organized as ctarget/schedule.h instead?

core/reactor_common.c Show resolved Hide resolved
core/tag.c Outdated Show resolved Hide resolved
core/tag.c Outdated Show resolved Hide resolved
core/tag.c Outdated Show resolved Hide resolved
core/tag.h Show resolved Hide resolved
core/tag.h Show resolved Hide resolved
include/ctarget_set.h Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Apr 26, 2022

Looks great! Thanks for cleaning up the C API.

Superficially, should files like ctarget_schedule.h be organized as ctarget/schedule.h instead?

I've been critical of the move to a separate ctarget_schedule.h because I think it looks weird, but I like your suggestion of having a ctarget/schedule.h. I think that might be a satisfying solution for everybody.

@housengw housengw requested a review from edwardalee April 26, 2022 16:43
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 is too big for me to go through in detail right now, but it looks good. What is the plan for updating the tests? Ideally, they all pass unchanged (backward compatibility), but then we update them so that they serve as proper examples. We also need to update the website code and docs.

@housengw
Copy link
Contributor Author

housengw commented Apr 26, 2022

My plan is to update the tests after merging this PR and lf-lang/lingua-franca#1097.
I will also help with updating the website code.

In regards to Marten's concern, I can do the renaming in a future PR. I think the most time-sensitive actions right now is to get the API out and to update the tests and the website

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.

Simplifying the SET API and its variants
4 participants