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

mark readonly parameters as const #86

Merged
merged 3 commits into from
Apr 29, 2019
Merged

mark readonly parameters as const #86

merged 3 commits into from
Apr 29, 2019

Conversation

recp
Copy link
Owner

@recp recp commented Apr 28, 2019

see #83

@coveralls
Copy link

coveralls commented Apr 28, 2019

Coverage Status

Coverage remained the same at 11.081% when pulling 14f06a2 on const into 120ae9a on master.

@acoto87
Copy link
Contributor

acoto87 commented Apr 29, 2019

I think it looks good. It's true that parameters that aren´t modified should be const to allow library users have const if they want.

How would this affect the struct implementation? Should struct parameters that aren´t modified be const as well?

@recp
Copy link
Owner Author

recp commented Apr 29, 2019

@acoto87 thanks for the review.

How would this affect the struct implementation? Should struct parameters that aren´t modified be const as well?

Yes I think it should be const, to prevent struct be copied in C++, we could do something like this:

CGLM_INLINE
mat4s
glm_mat4_mul(const mat4s CPP_REF a, const mat4s CPP_REF b)

it is similar to my comment (#83 (comment)) but alternative.

#ifdef __cplusplus
#define CPP_REF &
#else
#define CPP_REF
#endif

or maybe we keep focus on C and do this (C++ ref) in the future if needed

@acoto87
Copy link
Contributor

acoto87 commented Apr 29, 2019

Well it's your call if you want to support C++ features, but I think it would be better at the moment to keep it simple.

Yes I think it should be const, to prevent struct be copied in C++, we could do something like this:

Prevent copies when struct arguments are passed to functions is something you want to accomplish? In C the only way we have to do that is with pointers, right? Is that something you want to consider?

@recp
Copy link
Owner Author

recp commented Apr 29, 2019

It would be better to not copy structs if it is possible (it is possible in C++), since our structs are inline maybe compiler may not copy them, I'm not sure for certain. Personally I don't want to use pointers in struct params yet. Should it be pointer?

Let's keep it simple as you said for now and ignore what I said for C++, in the future we may support to move/ref if we want, it is not topic of today.

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

Successfully merging this pull request may close these issues.

None yet

3 participants