Skip to content

Conversation

@LingaoM
Copy link
Contributor

@LingaoM LingaoM commented Oct 31, 2024

  1. Use IS_ENABLED replace #if defined to make code more readable
  2. Move adv_tag_to_str to send_pending_adv, since only this used.

Comment on lines 75 to 76
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure parentheses are not needed here? Ternary operator has lower precedence than bitwise OR: https://en.cppreference.com/w/c/language/operator_precedence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, maybe look preprocessing .i file, i still don't know how to view the .i file compiled by zephyr. But add parentheses always right.

@LingaoM LingaoM force-pushed the tiny_enhance_adv_ext branch 2 times, most recently from 44f76d3 to 01a380c Compare October 31, 2024 07:25
1. Use IS_ENABLED replace #if defined to make code more readable
2. Move adv_tag_to_str to send_pending_adv, since only this used.

Signed-off-by: Lingao Meng <[email protected]>
@LingaoM LingaoM force-pushed the tiny_enhance_adv_ext branch from 01a380c to 68a64e7 Compare October 31, 2024 07:31
Comment on lines -132 to +116
if (IS_ENABLED(CONFIG_BT_MESH_ADV_EXT_GATT_SEPARATE)) {
return &advs[ARRAY_SIZE(advs) - 1];
} else {
return &advs[0];
}
return IS_ENABLED(CONFIG_BT_MESH_ADV_EXT_GATT_SEPARATE) ?
&advs[ARRAY_SIZE(advs) - 1] : &advs[0];
Copy link
Member

Choose a reason for hiding this comment

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

I know you're a fan of compact code, however I really don't think this improves readability. The C ternary operator is useful in places where you can't use if () however I don't think it improves readability at all (IMO it does the contrary).

Comment on lines -123 to +110
if (!!(CONFIG_BT_MESH_RELAY_ADV_SETS)) {
return &advs[1];
} else {
return &advs[0];
}
return !!(CONFIG_BT_MESH_RELAY_ADV_SETS) ? &advs[1] : &advs[0];
Copy link
Member

Choose a reason for hiding this comment

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

Same here, and this is also out of scope of the PR, since it's talking about replacing #if and not if ()

@omkar3141
Copy link
Contributor

IMO the old code was more readable.

@LingaoM LingaoM closed this Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants