Attention is currently required from: laforge, pespin. falconia has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32034 ) Change subject: codec: add osmo_efr_check_sid() function ...................................................................... Patch Set 1: (3 comments) File src/codec/gsm660.c: https://gerrit.osmocom.org/c/libosmocore/+/32034/comment/68640fc0_ddd57eeb PS1, Line 271: static con @laforge: I checked the code produced by gcc (5.3.0, came with Slackware 14.2) with and without the static, and if I drop the static, the compiler generates silly code: the actual table is still in .rodata, but it isn't entered into the symbol table as a sid_code_word_bits local symbol, instead there is a stack allocation (the actual "object") and the compiler emits code (basically an inline memcpy equivalent) that copies the table from .rodata to the stack. Putting the static back in results in sane generated code: just the table in .rodata, with the sid_code_word_bits local symbol on it, no extra stack allocation or copying. Hence I argue for keeping the static. @pespin: you correctly interpreted my intent here. https://gerrit.osmocom.org/c/libosmocore/+/32034/comment/68ce2a90_b31f85ec PS1, Line 272: /* bit numbers relative to "pure" EFR frame beginning, : * not counting the signature bits. */ : 45, 46, 48, 49, 50, 51, 52, 53, 54, 55, : 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, : 66, 67, 68, 94, 95, 96, 98, 99, 100, 101, : 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, : 112, 113, 114, 115, 116, 117, 118, 148, 149, 150, : 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, : 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, : 171, 196, 197, 198, 199, 200, 201, 202, 203, 204, : 205, 206, 207, 208, 209, 212, 213, 214, 215, 216, : 217, 218, 219, 220, 221 }; I was going for the same code structure and style as in osmo_fr_check_sid() in gsm610.c, but I agree that the code looks better and more stylistically correct with one less tab of indentation. I'll be spinning the next patch version with the reduced indent. https://gerrit.osmocom.org/c/libosmocore/+/32034/comment/e684efdf_ddf89716 PS1, Line 293: for (i = 0; i < ARRAY_SIZE(sid_code_word_bits); i++) Once again I was copying the code structure and style from osmo_fr_check_sid() and osmo_hr_check_sid(), where the same construct appears without the braces. But following the principle that newly added code should be styled "right", I am changing the construct in question to the braced style in the next patch version. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32034 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Iab9fb60028f4135375287bc42f5da7ca7838b5f0 Gerrit-Change-Number: 32034 Gerrit-PatchSet: 1 Gerrit-Owner: falconia &lt;falcon(a)freecalypso.org&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Comment-Date: Fri, 24 Mar 2023 23:07:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt; Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: comment