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

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

Conversation

x3ro
Copy link
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 added a commit to x3ro/RIOT that referenced this pull request Oct 30, 2017
_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
@miri64
Copy link
Member

miri64 commented Oct 30, 2017

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
Copy link
Member

miri64 commented Nov 1, 2017

Ping @x3ro?

@kYc0o
Copy link
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
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
Member

miri64 commented Nov 6, 2017

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
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
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
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
Contributor

kYc0o commented Nov 6, 2017

Ok great! I'll try to help.

x3ro added a commit to x3ro/RIOT that referenced this pull request Nov 7, 2017
_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
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 RIOT-OS#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
Contributor Author

x3ro commented Nov 7, 2017

Done

@miri64
Copy link
Member

miri64 commented Nov 7, 2017

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

@@ -290,6 +290,9 @@ void _nib_nc_get(const _nib_onl_entry_t *node, gnrc_ipv6_nib_nc_t *nce)
return;
}
}
#else
/* Prevent unused function error thrown by clang */
(void)_get_l2addr_from_ipv6;
Copy link
Member

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
Member

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
Copy link
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
_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
_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
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
Development

Successfully merging this pull request may close these issues.

4 participants