Skip to content

tests/gnrc_ipv6_nib: Fix unused function error w/ clang/macOS#7910

Merged
miri64 merged 1 commit intoRIOT-OS:gnrc_netif2_integration/masterfrom
x3ro:fix-gnrc-ipv6-nib-test-on-macos
Nov 7, 2017
Merged

tests/gnrc_ipv6_nib: Fix unused function error w/ clang/macOS#7910
miri64 merged 1 commit intoRIOT-OS:gnrc_netif2_integration/masterfrom
x3ro:fix-gnrc-ipv6-nib-test-on-macos

Conversation

@x3ro
Copy link
Copy Markdown
Contributor

@x3ro x3ro commented Oct 30, 2017

In the case that GNRC_IPV6_NIB_CONF_ARSM is set but
GNRC_IPV6_NIB_CONF_6LN is not, clang complains about
the function _get_l2addr_from_ipv6 never being used.
I couldn't easily figure out why this passes in Murdock,
but I'm guessing that clang is simply being smarter than
GCC. Can someone comment on whether there is a better fix
for this?

Relates to #6473

@x3ro x3ro added OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 30, 2017
@x3ro x3ro requested a review from miri64 October 30, 2017 11:38
@x3ro x3ro mentioned this pull request Oct 30, 2017
Merged
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 30, 2017 •
edited
Loading

In accordance to refactoring work on GNRC and the current merge embargo for GNRC, can you rebase this branch against gnrc_netif2_integration/master and change this PR to that base branch, please?

@x3ro x3ro changed the base branch from master to gnrc_netif2_integration/master October 30, 2017 14:54
@x3ro x3ro changed the base branch from gnrc_netif2_integration/master to master October 30, 2017 14:55
@miri64 miri64 self-assigned this Nov 1, 2017
@miri64 miri64 changed the base branch from master to gnrc_netif2_integration/master November 1, 2017 08:59
@miri64 miri64 mentioned this pull request Nov 1, 2017
Merged
28 tasks
@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 1, 2017

Ping @x3ro?

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Nov 6, 2017

This means that this PR cannot be solved for master until your working branch is merged?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 6, 2017

No. Since these are related to #7925 (and actual bugfixes to the NIB tests) I suggest to merge them into #7925. The base-branch is already adjusted. @x3ro just needs to rebase to that PRs branch.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 6, 2017 •
edited
Loading

This means that this PR cannot be solved for master until your working branch is merged?

but in general yes! There currently is a merge embargo on GNRC [edit]in master (this is why the review and testing of all PRs related to #7925 should have been started last week)[/edit]. Please read my mail on devel regarding this.

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Nov 6, 2017

I know about the embargo, just wanted to know if a "small" change to fix a bug in the existing implementation would remain a bug until the new netif gets merged. Thus, I guess that we will live with this at least until the next release.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 6, 2017

I know about the embargo, just wanted to know if a "small" change to fix a bug in the existing implementation would remain a bug until the new netif gets merged.

No, not even bugfixes (to master, but again: this PR is not supposed to go into master). The point is to keep the chance of merge conflicts as low as possible.

Thus, I guess that we will live with this at least until the next release.

Well the plan was, that the netif stuff is merged into master again end of this week, but since no reviews happend last week (and we are thus 3 days after schedule) I really don't think that will be the case.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 6, 2017

Well the plan was, that the netif stuff is merged into master again end of this week, but since no reviews happend last week (and we are thus 3 days after schedule) I really don't think that will be the case.

Let's cross fingers for end of next week ;-)

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Nov 6, 2017

Ok great! I'll try to help.

@x3ro
WIP tests/gnrc_ipv6_nib: Fix unused function error w/ clang/macOS
069849f 
In the case that GNRC_IPV6_NIB_CONF_ARSM is set but
GNRC_IPV6_NIB_CONF_6LN is not, clang complains about
the function _get_l2addr_from_ipv6 never being used.
I couldn't easily figure out why this passes in Murdock,
but I'm guessing that clang is simply being smarter than
GCC. Can someone comment on whether there is a better fix
for this?

Relates to #6473
@x3ro x3ro force-pushed the fix-gnrc-ipv6-nib-test-on-macos branch from 45cf1b0 to 069849f Compare November 7, 2017 16:36
@x3ro
Copy link
Copy Markdown
Contributor Author

x3ro commented Nov 7, 2017

Done

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 7, 2017

@x3ro Is there a reason for the WIP in the title?

miri64
miri64 requested changes Nov 7, 2017
View reviewed changes
sys/net/gnrc/network_layer/ipv6/nib/_nib-internal.c
}
#else
/* Prevent unused function error thrown by clang */
(void)_get_l2addr_from_ipv6;
Copy link
Copy Markdown
Member

@miri64 miri64 Nov 7, 2017

Choose a reason for hiding this comment

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

Can you put the _get_l2addr_from_ipv6() function into GNRC_IPV6_NIB_CONF_6LN conditional as well, instead, please?

Copy link
Copy Markdown
Member

@miri64 miri64 Nov 7, 2017

Choose a reason for hiding this comment

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

OK. I reallize now that this this is not possible, since it is also used in the else case of the GNRC_IPV6_NIB_CONF_ARSM case. Then let's go with this.

@miri64 miri64 changed the title WIP tests/gnrc_ipv6_nib: Fix unused function error w/ clang/macOS tests/gnrc_ipv6_nib: Fix unused function error w/ clang/macOS Nov 7, 2017
miri64
miri64 approved these changes Nov 7, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK

@miri64 miri64 merged commit b1e69a7 into RIOT-OS:gnrc_netif2_integration/master Nov 7, 2017
bergzand pushed a commit to bergzand/RIOT that referenced this pull request Nov 17, 2017
@x3ro @bergzand
tests/gnrc_ipv6_nib_6ln: Fix error w/ clang on macOS
b6421e0 
_is_reachable is only used when GNRC_IPV6_NIB_CONF_ARSM
is enabled, and as such it must be guarded so that clang
doesn't complain about a unused function in case
GNRC_IPV6_NIB_CONF_ARSM is not set

Similar to RIOT-OS#7910
Relates to 6473
bergzand pushed a commit to bergzand/RIOT that referenced this pull request Nov 19, 2017
@x3ro @bergzand
tests/gnrc_ipv6_nib_6ln: Fix error w/ clang on macOS
549565e 
_is_reachable is only used when GNRC_IPV6_NIB_CONF_ARSM
is enabled, and as such it must be guarded so that clang
doesn't complain about a unused function in case
GNRC_IPV6_NIB_CONF_ARSM is not set

Similar to RIOT-OS#7910
Relates to 6473
@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

Assignees

@miri64 miri64

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system

Projects

None yet

Milestone

Release 2018.01

Development

Successfully merging this pull request may close these issues.

4 participants

@x3ro @miri64 @kYc0o @aabadie