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

OSL_ALLOCA #1589

Merged
merged 2 commits into from
Sep 27, 2022
Merged

OSL_ALLOCA #1589

merged 2 commits into from
Sep 27, 2022

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Sep 27, 2022

We use OIIO_ALLOCA in many places. Make an OSL version and put it in platform.h.

Signed-off-by: Larry Gritz lg@larrygritz.com

We use OIIO_ALLOCA in many places. Make an OSL version and put it
in platform.h.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
/// things of bounded size, but not for anything that could be arbitrarily
/// big, which should instead use heap allocation.
#if defined(__GNUC__)
# define OSL_ALLOCA(type, size) (assert(size < (1<<20)), (size) != 0 ? ((type*)__builtin_alloca((size) * sizeof(type))) : nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any style guidelines here for casting? Should it be
(reinterpret_cast<type*>(__builtin_alloca((size) * sizeof(type))))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I am just an old programmer whose fingers tend to reflexively use C casts and forget about the C++ style until somebody points out my error. To be honest, I don't really feel that there are many places where the more verbose C++ syntax confers any advantage in practice.

I understand their purpose and see the point: const_cast only casts away constness while changing nothing else, static_cast is a value conversion from one type to another, reinterpret_cast turns a pointer into a different pointer type (but referring to the same memory). The use of the right specialized cast is a semantic breadcrumb conveying intent, whereas the C (cast) style is just a big hammer that clobbers everything. But the return value of alloca is already just raw memory that is semantic-free, so there's nothing to clobber, it's a truly arbitrary cast. In this case, what does reinterpret_cast<type*> do or what information does it convey that (type*) does not?

I'm just thinking out loud. I have no objection to the suggestion to change this to more idiomatic C++.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I don't have any objection to the old C cast, just using C++ as a matter of habit and was curious if there was a style guideline in practice for the project. Although I think I like the in your face verbosity of the reinterpret_cast<>() and the wrapping parenthesis around the item to cast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did update with the new style cast

Copy link
Contributor

@AlexMWells AlexMWells left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz lgritz merged commit b5aba16 into AcademySoftwareFoundation:main Sep 27, 2022
@lgritz lgritz deleted the lg-alloca branch September 28, 2022 01:40
lgritz added a commit to lgritz/OpenShadingLanguage that referenced this pull request Oct 10, 2022
We use OIIO_ALLOCA in many places. Make an OSL version and put it
in platform.h.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
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

4 participants