Change in libosmocore[master]: add contrib/struct_endianess.py

[Neels Hofmeyr]

Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11786 )

Change subject: add contrib/struct_endianess.py
......................................................................


Patch Set 2:

(6 comments)

more replies in the form of comments in the next patch set... in general, let's cut the time spent here (at least sysmocom time) since we don't really have a need to support big endian. Free time is a different matter.

https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py
File contrib/struct_endianess.py:

https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@80
PS2, Line 80:     # now the interesting part, go over the defs, and reverse the sub-byte ints
> You can probably split into a different function starting from here.
you're right style wise, but it doesn't really have a practical benefit


https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@118
PS2, Line 118:                     raise Exception('sub-byte int breaks clean byte bounds: %s -- %d + %d = %d bits'
> what about eg. […]
Even if you use uint16_t, it must still reverse at byte boundaries.

Technically we parse any integer types, including uint16_t, but this is identical:

  uint8_t a:3, b:5;

and

  uint16_t a:3, b:5;

and in practice *all* of our sub-byte integers today are defined by uint8_t.


https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@189
PS2, Line 189:         if not 'packed' in struct[-1]:
> probably better/easier to look for __atribute((packed)) or whatever is called, and check  all lines  […]
the regex for top-level structs only matches the end-of-struct being at the line start

  };

or

  } __attribute....;

This condition only exists because in some places we have weird booleans in the form of

  integer flag:1;

and these don't add up to bytes, and cause errors. So I thought let's skip all non-packed structs, then all of those are out of the picture.

For the current code base that holds true...


https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@248
PS2, Line 248:                         j += 1
> Not sure if this j += 1 in here applied for all cases is correct. Have a look.
it is correct, all ifs break, this is the iteration case


https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@276
PS2, Line 276:             end_def()
> afaiu this end_def() should be one lne above and indented one more level (inside while).
nope. it flushes what is left at the end


https://gerrit.osmocom.org/#/c/11786/2/contrib/struct_endianess.py@308
PS2, Line 308:             raise Exception('do not know where to include osmocom/core/endian.h in %r' % f)
> Just add it around first empty line (to avoid adding if before guard IFDEF or pragma once).
there are currently no cases where this would be needed... am reluctant to add dead code



-- 
To view, visit https://gerrit.osmocom.org/11786
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e75b17d8071c7b3a2a171ba776fb76854b28a53
Gerrit-Change-Number: 11786
Gerrit-PatchSet: 2
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 19 Nov 2018 15:31:00 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181119/ac856678/attachment.htm>