<p><a href="https://gerrit.osmocom.org/11786">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py">File contrib/struct_endianess.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@37">Patch Set #2, Line 37:</a> <code style="font-family:monospace,monospace">def struct_body_to_big_endian(body_str):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Document what this function does exactly.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@80">Patch Set #2, Line 80:</a> <code style="font-family:monospace,monospace"> # now the interesting part, go over the defs, and reverse the sub-byte ints</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You can probably split into a different function starting from here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@118">Patch Set #2, Line 118:</a> <code style="font-family:monospace,monospace"> raise Exception('sub-byte int breaks clean byte bounds: %s -- %d + %d = %d bits'</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what about eg. bitfield for uint16_t? are they expected to be reversed too in big endian?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@168">Patch Set #2, Line 168:</a> <code style="font-family:monospace,monospace"> sections.append(buf)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">shouldn't appending to sections be done at the end of the struct only? (line 173).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@181">Patch Set #2, Line 181:</a> <code style="font-family:monospace,monospace"> # examine each struct, i.e. every second item in 'sections'</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ah I see now, the one you append is actually the buffer until before the struct code starts. Worth explicitly commenting that during first "for" loop I think, specially comment "why" you want to have both at that point.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@189">Patch Set #2, Line 189:</a> <code style="font-family:monospace,monospace"> if not 'packed' in struct[-1]:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">probably better/easier to look for __atribute((packed)) or whatever is called, and check all lines (to avoid possible corner cases with whitespace or whatever).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@199">Patch Set #2, Line 199:</a> <code style="font-family:monospace,monospace"> # divide in struct body sections of</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Probably worth moving this into another function to make code easier to read and maintain.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@200">Patch Set #2, Line 200:</a> <code style="font-family:monospace,monospace"> # ['arbitrary string', ['body;\n', 'lines;\n'], 'arbitrary string']</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what does body and lines means? brief explanation here welcome.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@208">Patch Set #2, Line 208:</a> <code style="font-family:monospace,monospace"> def end_def():</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please document this function and global variables below.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@224">Patch Set #2, Line 224:</a> <code style="font-family:monospace,monospace"> or re_substruct_end.fullmatch(line)):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this can go in same line</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@248">Patch Set #2, Line 248:</a> <code style="font-family:monospace,monospace"> j += 1</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure if this j += 1 in here applied for all cases is correct. Have a look.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@276">Patch Set #2, Line 276:</a> <code style="font-family:monospace,monospace"> end_def()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">afaiu this end_def() should be one lne above and indented one more level (inside while).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@308">Patch Set #2, Line 308:</a> <code style="font-family:monospace,monospace"> raise Exception('do not know where to include osmocom/core/endian.h in %r' % f)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Just add it around first empty line (to avoid adding if before guard IFDEF or pragma once).</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11786">change 11786</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/11786"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I8e75b17d8071c7b3a2a171ba776fb76854b28a53 </div>
<div style="display:none"> Gerrit-Change-Number: 11786 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 16 Nov 2018 10:41:23 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>