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.

Sign up for GitHub

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

Jump to bottom

icmpv6: remove superfluous header build functions #3693

Merged
merged 1 commit into from
Oct 27, 2015
Merged

icmpv6: remove superfluous header build functions #3693

merged 1 commit into from
Oct 27, 2015

Conversation

OlegHahm
Copy link
Member

These functions seem unnecessary and only one of them was actually used at only one place.

@OlegHahm OlegHahm added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Aug 22, 2015
@OlegHahm OlegHahm mentioned this pull request Aug 22, 2015
Merged
@miri64
Copy link
Member

miri64 commented Aug 22, 2015

Mh… I don't like it, they are after all inline-functions, and as a bonus help people, that don't know much about the headers to build them.

@miri64
Copy link
Member

miri64 commented Aug 22, 2015

Can you show me a bin-diff, that this actually brings any advantage?

@OlegHahm
Copy link
Member Author

I argue that these inline functions bring any advantage for no-one.

@OlegHahm
Copy link
Member Author

This PR tries not optimize code size but keep the API slim and I don't see any advantage in a do_foo_xyz(..-) function as a wrapper for do_foo(xyz, ...).

@miri64
Copy link
Member

miri64 commented Aug 22, 2015

I see it, because it is convenient (at least for me).

@miri64
Copy link
Member

miri64 commented Aug 22, 2015

If we do it your way, gnrc_icmpv6_echo_build() would need a check if the type is correct btw.

@OlegHahm
Copy link
Member Author

I don't see the necessity for that. Whoever calls this function should not what she/he's doing.

@miri64
Copy link
Member

miri64 commented Aug 25, 2015

I think this is a point of contention between us. If you want this merged, I think you have to reassign :D

@cgundogan
Copy link
Member

Did anyone of you had a change of heart since the last discussion? Personally, I also would let go of the inlined wrapper functions and opt for a slimmer API. So ACK from my side

@cgundogan cgundogan added Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 27, 2015
@miri64
Copy link
Member

miri64 commented Oct 27, 2015

I still think it is inconvenient, but since it seems that I'm overruled let's do it.

@cgundogan
Copy link
Member

travis approves and everybody seems to be happy. GO

cgundogan added a commit that referenced this pull request Oct 27, 2015
@cgundogan
Merge pull request #3693 from OlegHahm/remove_superfluous_icmpv6_hdr_…
c175273 
…build_functions

icmpv6: remove superfluous header build functions
@cgundogan cgundogan merged commit c175273 into RIOT-OS:master Oct 27, 2015
@OlegHahm OlegHahm deleted the remove_superfluous_icmpv6_hdr_build_functions branch October 27, 2015 19:06
@miri64
Copy link
Member

miri64 commented Oct 27, 2015

I'm not happy, I'm just complying to an argument I'm tired of having :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@miri64 miri64

Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@OlegHahm @miri64 @cgundogan