On Wed, 2023-01-18 at 16:36 -0800, Jakub Kicinski wrote:
>
> +class BaseNlLib:
> + def __init__(self):
> + pass
That __init__ seems pointless?
> + self.name = attr['name'].replace('-', '_')
You have a _lot_ of these ".replace('-', '_')" invocations - maybe make
a function just like you have c_upper()/c_lower()?
> + if self.presence_type() == 'len':
> + pfx = '__' if space == 'user' else ''
this is how you define pfx
> + if member:
> + ptr = ''
> + if self. is_multi_val():
> + ptr = '*'
but this is how you define ptr - feels a bit inconsistent
Also note the extra space after "self." there
> + if not self.is_multi_val():
> + ri.cw.p(f"if (ynl_attr_validate(yarg, attr))")
> + ri.cw.p(f"return MNL_CB_ERROR;")
Those didn't need f-strings.
Does this "ri.cw.p()" do indentation for the code?
> + def attr_policy(self, cw):
> + if 'unterminated-ok' in self.checks and self.checks['unterminated-ok']:
maybe
if self.checks.get('unterminated-ok', 0):
> +class TypeBinary(Type):
> + def arg_member(self, ri):
> + return [f"const void *{self.c_name}", 'size_t len']
> +
> + def presence_type(self):
> + return 'len'
> +
> + def struct_member(self, ri):
> + ri.cw.p(f"void *{self.c_name};")
> +
> + def _attr_typol(self):
> + return f'.type = YNL_PT_BINARY,'
> +
> + def _attr_policy(self, policy):
> + mem = '{ '
> + if len(self.checks) == 1 and 'min-len' in self.checks:
> + mem += '.len = ' + str(self.checks['min-len'])
Why does the len(self.checks) matter?
> + elif len(self.checks) == 0:
> + mem += '.type = NLA_BINARY'
> + else:
> + raise Exception('Binary type not implemented, yet')
to get to that error messsage? The error messsage seems a bit misleading
too though. It's that 'max-len' isn't implemented, or such, no?
> + def _complex_member_type(self, ri):
> + if 'type' not in self.attr or self.attr['type'] == 'nest':
could do
if self.attr.get('type', 'nest') == 'nest':
again like above
> + return f"struct {self.nested_render_name}"
> + elif self.attr['type'] in scalars:
> + scalar_pfx = '__' if ri.ku_space == 'user' else ''
You also have this
... = '__' if ... == 'user' else ''
quite a few times, maybe add something like
def qualify_user(value, user):
if user == 'user':
return '__' + value
return value
> + return scalar_pfx + self.attr['type']
and then this is just
return qualify_user(self.attr['type'], ri.ku_space)
instead of the two lines
> + def free_needs_iter(self):
> + return 'type' not in self.attr or self.attr['type'] == 'nest'
> +
> + def free(self, ri, var, ref):
> + if 'type' not in self.attr or self.attr['type'] == 'nest':
two more places that could use the .get trick
but it's really up to you. Just that the line like that seems rather
long to me :-)
There are many more below but I'll stop commenting.
> + if self.c_name in c_kw:
> + self.c_name += '_'
You also have this pattern quite a lot. Maybe a "c_safe()" wrapper or
something :)
FWIW, I'm still looking for "c_kw", maybe "pythonically" that should be
"_C_KW" since it's a private global? I think? Haven't seen it yet :)
> +c_kw = {
> + 'do'
> +}
Aha, here it is :-)
> + if has_ntf:
> + cw.p('// --------------- Common notification parsing --------------- //')
You said you were using /* */ comments now but this is still there.
> + print_ntf_parse_prototype(parsed, cw)
> + cw.nl()
> + else:
> + cw.p('// Policies')
and here too, etc.
Whew. I think I skipped some bits ;-)
Doesn't look that bad overall, IMHO. :)
johannes
