Skip to content

controller/adv: fix 'unused function' warning#678

Merged
andrzej-kaczmarek merged 1 commit intoapache:masterfrom
haukepetersen:fix_controller_activechansetisprivunsused
Dec 16, 2019
Merged

controller/adv: fix 'unused function' warning#678
andrzej-kaczmarek merged 1 commit intoapache:masterfrom
haukepetersen:fix_controller_activechansetisprivunsused

Conversation

@haukepetersen
Copy link
Copy Markdown
Member

When compiling without the LL_EXT_ADV feature and with disabled
assertions, the compiler throws an 'unused function' warning for
the internal function ble_ll_adv_active_chanset_is_pri(). This
commit fixes this by only compiling that functions if actually
needed.

#define SYNC_DATA_LEN(_advsm) \
(_advsm->periodic_adv_data ? OS_MBUF_PKTLEN(advsm->periodic_adv_data) : 0)

#if defined (BLE_LL_CFG_FEAT_LL_EXT_ADV) || !defined(NDEBUG)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should be MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_EXT_ADV) here

@sjanc
Copy link
Copy Markdown
Contributor

sjanc commented Nov 22, 2019

I have a bit of mixed feelings about this !define(NDEBUG), while in short term it is fine maybe in long term we should always use BLE_LL_ASSERT() and evaluate parameters but make it dead code for NDEBUG...

@andrzej-kaczmarek
Copy link
Copy Markdown
Contributor

we could also remove all those asserts for is_pri since they were primarily to debug things when adv code was refactored. I did not see any of them actually trigger an error in long long time.

@haukepetersen
Copy link
Copy Markdown
Member Author

I share the same feeling about NDEBUG, but it was the best I could think of right now...

+1 for simply removing the asserts. @sjanc do you agree as well? If yes, I will adapt this PR.

@haukepetersen
Copy link
Copy Markdown
Member Author

so how does this look?

@sjanc
Copy link
Copy Markdown
Contributor

sjanc commented Nov 22, 2019

yeap, seems fine

@haukepetersen haukepetersen force-pushed the fix_controller_activechansetisprivunsused branch from 6f4219a to 5d71b24 Compare November 22, 2019 18:15
@haukepetersen
Copy link
Copy Markdown
Member Author

perfect, squashed just in case @andrzej-kaczmarek likes it as well :-)

@andrzej-kaczmarek
Copy link
Copy Markdown
Contributor

try to merge or let me know in case you can't do it, I'll merge then

@haukepetersen
Copy link
Copy Markdown
Member Author

I am still not allowed to merge, so please go ahead.

Is there maybe anyone else I should contact regarding the repository permissions?

@rymanluk
Copy link
Copy Markdown
Contributor

rymanluk commented Dec 5, 2019

@haukepetersen I gave you instruction whom to contact on the slack

When compiling without the LL_EXT_ADV feature and with disabled
assertions, the compiler throws an 'unused function' warning for
the internal function `ble_ll_adv_active_chanset_is_pri()`. This
commit fixes this by only compiling that functions if actually
needed and removing some assertions that have never triggered.
@haukepetersen haukepetersen force-pushed the fix_controller_activechansetisprivunsused branch from 5d71b24 to c9255c6 Compare December 16, 2019 09:46
@haukepetersen
Copy link
Copy Markdown
Member Author

rebased. Still working on getting the commit permissions - was occupied elsewhere the last weeks...

@andrzej-kaczmarek andrzej-kaczmarek merged commit 946cfb7 into apache:master Dec 16, 2019
@haukepetersen haukepetersen deleted the fix_controller_activechansetisprivunsused branch December 16, 2019 11:23
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.

4 participants