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

Add sets to cwrapper #1845

Merged
merged 1 commit into from Nov 2, 2021
Merged

Add sets to cwrapper #1845

merged 1 commit into from Nov 2, 2021

Conversation

rikardn
Copy link
Contributor

@rikardn rikardn commented Sep 23, 2021

No description provided.

@rikardn
Copy link
Contributor Author

rikardn commented Sep 26, 2021

This is ready for review

@rikardn rikardn requested a review from a team September 26, 2021 16:44
@@ -658,6 +662,164 @@ void basic_str_free(char *s)
delete[] s;
}

void bool_true(basic s)
Copy link
Member

@isuruf isuruf Sep 30, 2021

Choose a reason for hiding this comment

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

basic_assign_bool_true?

Copy link
Member

Choose a reason for hiding this comment

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

Or basic_bool_set_true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, we have integer_set_si so I changed to bool_set_true and bool_set_false

@Marlin-Na
Copy link
Member

I wonder if we should prefix all/most functions with basic or basic_set. E.g. functions like is_subset sounds easy to collide with other libraries or user code.

@rikardn
Copy link
Contributor Author

rikardn commented Oct 7, 2021

Thanks @Marlin-Na for reviewing. I agree. Perhaps even better would be to have a symengine prefix then class and then function. Something like symengine_set_is_subset or se_set_is_subset, but this would go against the current naming scheme. Otherwise I think set_is_subset is the closest to the current naming scheme. What do you think?

@rikardn
Copy link
Contributor Author

rikardn commented Oct 7, 2021

I changed to set_is_subset etc. Let me know if you think something else would be better.

@Marlin-Na
Copy link
Member

@rikardn Thanks for making those changes. I still think having a basic_ prefix makes sense for all functions that return basic struct, which includes set. However, given we already have exceptions with integer, real, complex etc, I think the set_ prefix should be acceptable.

@rikardn
Copy link
Contributor Author

rikardn commented Oct 17, 2021

Can this be merged?

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

I agree with @Marlin-Na that set_* is too common. Let's add basic_ prefix. Otherwise looks good to me.

@rikardn
Copy link
Contributor Author

rikardn commented Oct 31, 2021

Ok. Is this better?

@rikardn
Copy link
Contributor Author

rikardn commented Nov 2, 2021

Thanks @isuruf and @Marlin-Na for reviewing

@rikardn rikardn merged commit 9f1be73 into symengine:master Nov 2, 2021
@rikardn rikardn deleted the cwrapsets branch November 2, 2021 07:24
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.

None yet

3 participants