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

Fix LF_ASSERT #324

Merged
merged 2 commits into from
Dec 28, 2023
Merged

Fix LF_ASSERT #324

merged 2 commits into from
Dec 28, 2023

Conversation

erlingrj
Copy link
Collaborator

Thanks @edwardalee for spotting this mistake. LF_ASSERT was not expanded correctly when we used build-type: Release.

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 change is necessary to ensure correctness, and the test failures are just the usual macOS federated failures that all branches are getting. However, I think a better solution is needed for the long run with more customized macros (e.g. for checking memory allocation). This change will likely result in compiler warnings about the result of expressions not being used when using build-type: Release.

@erlingrj
Copy link
Collaborator Author

I am not getting compiler warnings when compiling with build-type: Release. We can make more elaborate macros also. But I think the LF_ASSERT macro still has its use.

@erlingrj erlingrj merged commit fa07f2f into main Dec 28, 2023
25 of 28 checks passed
@edwardalee
Copy link
Contributor

Unfortunately, with this change, I get hundreds of warnings as follows:

/Users/edwardlee/LinguaFranca/test/C/src-gen/modal_models/ConvertCaseTest/core/environment.c:209:20: warning: expression result unused [-Wunused-value]
    LF_ASSERT(env->shutdown_reactions, "Out of memory");

We need a different solution.

@erlingrj
Copy link
Collaborator Author

Unfortunate that I am not getting them. Will see if I can turn those warnings on. Probably solvable with a void cast. Give me a sec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants