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

Expose simulate_until to C API #325

Merged
merged 10 commits into from
Sep 4, 2019
Merged

Conversation

markaren
Copy link
Contributor

No description provided.

src/c/cse.cpp Outdated Show resolved Hide resolved
include/cse.h Show resolved Hide resolved
src/c/cse.cpp Show resolved Hide resolved
src/c/cse.cpp Show resolved Hide resolved
@markaren
Copy link
Contributor Author

markaren commented Sep 3, 2019

How about this PR now?

@kyllingstad
Copy link
Member

I'm so sorry, it turns out I have completely misled you here.

I thought cse::execution::simulate_until() returned a cse::step_result, but it doesn't. Instead, it returns a boolean that specifies whether the simulation actually progressed to the requested target time. A false return value does not indicate error, it only means that cse::execution::stop_simulation() was called.

So cse_execution_simulate_until() should probably also return a boolean. C doesn't have a boolean type1, one just uses integers 0 and 1. So the function would return -1, 0 or 1 for error, false or true, respectively. Defining CSE_FALSE and CSE_TRUE constants in cse.h might be a good idea.

1 At least not the version of C that is a subset of C++. Newer versions have the <stdbool.h> header, but we should probably stay clear of that.

@markaren
Copy link
Contributor Author

markaren commented Sep 3, 2019

How about the 2 last commits?

Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

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

Great!

The CSE_FALSE and CSE_TRUE constants will also come in handy when we add support for boolean variables to the C header. :)

@eidekrist
Copy link
Member

So cse_execution_simulate_until() should probably also return a boolean. C doesn't have a boolean type1, one just uses integers 0 and 1. So the function would return -1, 0 or 1 for error, false or true, respectively. Defining CSE_FALSE and CSE_TRUE constants in cse.h might be a good idea.

1 At least not the version of C that is a subset of C++. Newer versions have the <stdbool.h> header, but we should probably stay clear of that.

Hold your horses! We have already included and used the <stdbool.h> header in cse.h. Should that be removed again? Otherwise, CSE_FALSE and CSE_TRUE are redundant.

The CSE_FALSE and CSE_TRUE constants will also come in handy when we add support for boolean variables to the C header. :)

Hold your horses! We already have support for retrieving/observing and overriding boolean variables :)

@markaren
Copy link
Contributor Author

markaren commented Sep 4, 2019

So I should just return the bool I get, and remove CSE_TRUE/FALSE ?

@kyllingstad
Copy link
Member

kyllingstad commented Sep 4, 2019

@eidekrist:

Hold your horses!

:D My embarrassingly lacking knowledge of the cse.h header is becoming more and more apparent. Excuse me while I go find a dark corner in which to sit in shame.

We have already included and used the <stdbool.h> header in cse.h. Should that be removed again?

No, I guess it's fine. I considered that before replying to @markaren, but that header is not included in the "C subset" of C++1, so anyone trying to use the cse.h header from C++ might run into problems. Then again, let's not be completely reactionary and only stick to ancient C standards.

New suggestion, then: Use the true and false constants from <stdbool.h>, but wrap the #include directive like so:

#ifndef __cplusplus
#    include <stdbool.h>
#endif

(I know, that part is strictly unrelated to this PR, but we might as well fix it now.)

1 More precisely, the <stdbool.h> header from C99 and the corresponding <cstdbool> C++ header were introduced in C++11, deprecated in C++17, and removed in C++20. The reason is that they only define the bool type and the true and false constants, which are built-in keywords in C++.

@kyllingstad
Copy link
Member

Note, the simulate_until function should still return int because it also needs to be able to return -1.

@markaren
Copy link
Contributor Author

markaren commented Sep 4, 2019

Ok now?

Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

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

Reaffirming approval. :)

@markaren markaren merged commit a28ffd8 into master Sep 4, 2019
@markaren markaren deleted the expose-simulate-until-to-c-api branch September 4, 2019 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants