Array symbolic dimensions #186
Conversation
| # dim should not be None here | ||
| yield ' '.join(dim) | ||
| dim = None | ||
| elif dim is not None: |
There was a problem hiding this comment.
This is really just an else: right? I think it's better to do it that way. If due to some bug it tries to append something to None it will fail loudly instead of continuing silently.
Moreover, at 1st glance this made it look like the dims has a secondary function here as a way of signalling where we are in the token processing when in fact it isn't: we're either at the beginning [, the end ], or it's something to append. I think? I'm tired, so... 😅
There was a problem hiding this comment.
dims actually does have that secondary function! The tokens also contain stuff before the first [, and we want to ignore them. A quick debug print reveals it:
diff --git a/src/hawkmoth/parser.py b/src/hawkmoth/parser.py
index 49bcfe96db2a..a9b08cb8ad4c 100644
--- a/src/hawkmoth/parser.py
+++ b/src/hawkmoth/parser.py
@@ -427,6 +427,8 @@ def _normalize_type(type_string):
return 'bool' if type_string == '_Bool' else type_string
def _symbolic_dims(cursor):
+ print([t.spelling for t in _cursor_get_tokens(cursor)])
+
dim = None
for spelling in [t.spelling for t in _cursor_get_tokens(cursor)]:
if spelling == '[':
Running hawkmoth test/c/array-symbolic-dimensions.c with that on top produces e.g.:
['static', 'int', 'foo', '[', 'MACRO_SIZE', ']']
['static', 'int', 'bar', '[', 'MACRO_SIZE', '*', 'ENUM_SIZE', ']', '[', '3', ']']
There was a problem hiding this comment.
Ah, right! I guess I should have tested it.
Not sure I'm a fan, it may be a bit too clever. But tests are failing on my side (probably some update, I hadn't picked this up in a while), so I won't be volunteering any alternative right now.
It's quite readable anyway, it's 👍 from me.
There was a problem hiding this comment.
I'm always eager to merge after seeing the thumbs up... but just double checking, by "a bit too clever" you mean the details of the implementation (which can be improved later), not the idea?
There was a problem hiding this comment.
Hey, sorry I thought I had replied to this. Yeah, I meant the implementation with regards to the use of the dims variable and such. But again, it's totally readable so I think it's fine.
There was a problem hiding this comment.
Merged now anyway, thanks!
In preparation for upcoming changes, store array dimensions in a list while parsing types.
In most cases, magic values for array dimensions in documentation are meaningless, and the symbolic name in code is desirable. Try to parse the symbolic names from tokens.
jnikula
force-pushed
the
array-symbolic-dimensions
branch
from
55bbff0
to
07217cf
Compare
|
The force push was just a rebase because there were conflicts. |
Things like
get documented with the actual value 5 rather than the symbolic
SIZE. Fix it.(This includes #184 to ensure it works at all.)