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 aco_mem_new macro #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add aco_mem_new macro #23

wants to merge 1 commit into from

Conversation

platipo
Copy link

@platipo platipo commented Oct 7, 2018

I followed your coding convention, but I think that lowercase macros are evil because it confuses newcomers, uppercase macros are convention.

I took the freedom to add a new macro aco_mem_calloc which you can read more details in the commit.

Also I want to point out that this commit "breaks" C++ support, which was already broken by _Static_assert, because using malloc in C++ is a really bad habit and there is no support for typeof of something similar in C++.

Issue #3.

Two types of allocation were done in the code, the former is the allocation
to predefined size and the latter callocation of single object.
I added a new macro aco_mem_calloc to handle the second case. I used the
calloc insted of malloc + memset for brevity.

This commit "breaks" C++ support.
@platipo
Copy link
Author

platipo commented Oct 7, 2018

One workaround if you wanna stick with malloc could be:

# define aco_mem_new(ptr, size) do { \
    ptr = static_cast<decltype(ptr)>(malloc(size)); \
    assertalloc_ptr(ptr); \
} while (0);
# define aco_mem_calloc(ptr, count) do { \
    ptr = static_cast<decltype(ptr)>(calloc(count, sizeof(*ptr))); \
    assertalloc_ptr(ptr); \
} while (0);

instead of #error

@hnes
Copy link
Owner

hnes commented Oct 13, 2018

Thank you very much for this PR, @platipo :D

I am very sorry to reply so late.

I followed your coding convention, but I think that lowercase macros are evil because it confuses newcomers, uppercase macros are convention.

Yes, but I'm afraid that I only agree with you partly. In the Code Style of linux kernel there is:

CAPITALIZED macro names are appreciated but macros resembling functions may be named in lower case.

Which is also the convention been used in the libaco. In this case, the aco_mem_new(ptr, sz) in this PR may should be changed to the form of ptr = (type_cast) aco_mem_new(sz) and aco_mem_calloc should be similar. We may should also implement them as "static inline functions" because the statement expressions is not a part of C standard.

I took the freedom to add a new macro aco_mem_calloc which you can read more details in the commit.

I very appreciate your idea. For many situations, "calloc" is indeed usually more efficient than "malloc + memset".

Also I want to point out that this commit "breaks" C++ support, which was already broken by _Static_assert, because using malloc in C++ is a really bad habit and there is no support for typeof of something similar in C++.

I don't think the _Static_assert would break the C++ support because:

  • Our user should compile libaco with a C compiler since libaco is a C library.

  • C++ application should call libaco as a C library.

  • _Static_assert is a part of C11 (and which is also being supported by default as an extension in many mainstream C compilers like GCC and clang).

And finally, please forgive me that I would like to deal with this PR after the commit of issue #22 since it has the highest priority ;-)

@platipo
Copy link
Author

platipo commented Oct 13, 2018

We may should also implement them as "static inline functions" because the statement expressions is not a part of C standard.

I think that's the best thing to do.

Seeing lowercase macros annoys me because I have always seen capitalized macro names even in function like macros and whats really bothers me is macro expansion.
It's notorious that macros may affect things you don't realize and can lead to strange side effects, indeed we are using them as functions but they don't have return like behavior.
So I think its needed to emphasize their usage by al least uppercasing their names.

Our user should compile libaco with a C compiler since libaco is a C library.

If so why casting void pointers, it's not correct in C because they are automatically and safely promoted to any other pointer type.

And finally, please forgive me that I would like to deal with this PR after the commit of issue #22 since it has the highest priority ;-)

Of course 😃

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.

2 participants