-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for enums and attributes #13
base: master
Are you sure you want to change the base?
Changes from 3 commits
80606e4
c8e32cc
bd63c99
f3811d5
0240797
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,7 +136,7 @@ class DataType(Element): | |
""" | ||
Base class for all data types | ||
""" | ||
def __init__(self, name: str | None) -> None: | ||
def __init__(self, name: str = "") -> None: | ||
self.name = name | ||
|
||
|
||
|
@@ -150,7 +150,7 @@ def __init__(self, | |
pointer: bool = False, | ||
volatile: bool = False, | ||
array: int | None = None) -> None: # Only used for typedefs to other array types | ||
super().__init__(None) | ||
super().__init__("") | ||
self.base_type = base_type | ||
self.const = const | ||
self.volatile = volatile | ||
|
@@ -168,6 +168,45 @@ def qualifier(self, name) -> bool: | |
else: | ||
raise KeyError(name) | ||
|
||
class EnumMember(Element): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be named EnumValue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually not. It is like StructMember. This is also member of Enum type, which has name and value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another point of view: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about StructElement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well it is up to you. But it should be usable for both cases. I am coauthor of eRPC project (similar to this) and at the beginning i was lead by USA architect who decided to use StructMember and EnumMember. Sounds good to me. https://github.com/EmbeddedRPC/erpc/tree/develop/erpcgen/src/types There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be also StructItem and EnumItem |
||
""" | ||
Enum element. | ||
""" | ||
|
||
def __init__(self, | ||
name: str, | ||
value: int | None) -> None: | ||
self.name = name | ||
self.value = value | ||
|
||
|
||
class Enum(DataType): | ||
""" | ||
A enum definition | ||
""" | ||
|
||
def __init__(self, name: str, members: list[EnumMember] = [], attributes: list[str] = []) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You must never use empty list as default value in functions. It's dangerous. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? It is valid case, where you have no members. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. t's a very common mistake among newcomers to Python. Google it, there are great explanations on the internet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That just sad. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's safer to do a proper check against None.
These days however I always take it one step further and double-check that each individual element in the given argument list is of expected type. I usually do this by calling a helper function (usually called |
||
super().__init__(name) | ||
if isinstance(members, list): | ||
self.members: list[EnumMember] = members.copy() | ||
enumValue = -1 | ||
for enum in self.members: | ||
if enum.value == None: | ||
enumValue += 1 | ||
enum.value = enumValue | ||
else: | ||
enumValue = enum.value | ||
else: | ||
raise TypeError('Invalid argument type for "elements"') | ||
self.attributes = attributes.copy() | ||
|
||
def append(self, member: EnumMember) -> None: | ||
""" | ||
Appends new element to the struct definition | ||
""" | ||
if not isinstance(member, EnumMember): | ||
raise TypeError(f'Invalid type, expected "EnumMember", got {str(type(member))}') | ||
self.members.append(member) | ||
|
||
class StructMember(Element): | ||
""" | ||
|
@@ -197,17 +236,13 @@ class Struct(DataType): | |
""" | ||
A struct definition | ||
""" | ||
def __init__(self, name: str | None, members: StructMember | list[StructMember] | None = None) -> None: | ||
def __init__(self, name: str = "", members: list[StructMember] = [], attributes: list[str] = []) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never use empty list as initializer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, i just need this one so i implemented them this way. But i was thinking to have something like cfiles attribute class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, i just need this one so i implemented them this way. But i was thinking to have something like cfiles attribute class |
||
super().__init__(name) | ||
self.members: list[StructMember] = [] | ||
if members is not None: | ||
if isinstance(members, StructMember): | ||
self.append(members) | ||
elif isinstance(members, list): | ||
for member in members: | ||
self.append(member) | ||
else: | ||
raise TypeError('Invalid argument type for "elements"') | ||
if isinstance(members, list): | ||
self.members: list[StructMember] = members.copy() | ||
else: | ||
raise TypeError('Invalid argument type for "elements"') | ||
self.attributes = attributes.copy() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't use copy. If you want to create a new list which is a copy of another, write it as |
||
|
||
def append(self, member: StructMember) -> None: | ||
""" | ||
|
@@ -322,7 +357,7 @@ def __init__(self, | |
static: bool = False, | ||
const: bool = False, # const function (as seen in C++) | ||
extern: bool = False, | ||
params: Variable | list[Variable] | None = None) -> None: | ||
params: list[Variable] = []) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty list as default value |
||
self.name = name | ||
self.static = static | ||
self.const = const | ||
|
@@ -335,13 +370,7 @@ def __init__(self, | |
self.return_type = Type("void") | ||
else: | ||
raise TypeError(str(type(return_type))) | ||
self.params: list[Variable] = [] | ||
if params is not None: | ||
if isinstance(params, Variable): | ||
self.append(params) | ||
elif isinstance(params, list): | ||
for param in params: | ||
self.append(param) | ||
self.params = params.copy() | ||
|
||
def append(self, param: Variable) -> "Function": | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,17 +17,18 @@ class ElementType(Enum): | |
DIRECTIVE = 1 | ||
COMMENT = 2 | ||
TYPE_DECLARATION = 3 | ||
STRUCT_DECLARATION = 4 # Should this be separate from type declaration? | ||
VARIABLE_DECLARATION = 5 | ||
FUNCTION_DECLARATION = 6 | ||
TYPEDEF = 7 | ||
TYPE_INSTANCE = 8 | ||
STRUCT_INSTANCE = 9 | ||
VARIABLE_USAGE = 10 | ||
FUNCTION_CALL = 11 | ||
STATEMENT = 12 | ||
BLOCK_START = 13 | ||
BLOCK_END = 14 | ||
ENUM_DECLARATION = 4 # Should this be separate from type declaration? | ||
STRUCT_DECLARATION = 5 # Should this be separate from type declaration? | ||
VARIABLE_DECLARATION = 6 | ||
FUNCTION_DECLARATION = 7 | ||
TYPEDEF = 8 | ||
TYPE_INSTANCE = 9 | ||
STRUCT_INSTANCE = 10 | ||
VARIABLE_USAGE = 11 | ||
FUNCTION_CALL = 12 | ||
STATEMENT = 13 | ||
BLOCK_START = 14 | ||
BLOCK_END = 15 | ||
|
||
|
||
class Formatter: | ||
|
@@ -102,6 +103,7 @@ def __init__(self, style: c_style.StyleOptions) -> None: | |
self.switcher_all = { | ||
"Type": self._write_base_type, | ||
"TypeDef": self._write_typedef_usage, | ||
"Enum": self._write_enum_usage, | ||
"Struct": self._write_struct_usage, | ||
"Variable": self._write_variable_usage, | ||
"Function": self._write_function_usage, | ||
|
@@ -280,6 +282,8 @@ def _write_declaration(self, elem: core.Declaration) -> None: | |
self._write_type_declaration(elem.element) | ||
elif isinstance(elem.element, core.TypeDef): | ||
self._write_typedef_declaration(elem.element) | ||
elif isinstance(elem.element, core.Enum): | ||
self._write_enum_declaration(elem.element) | ||
elif isinstance(elem.element, core.Struct): | ||
self._write_struct_declaration(elem.element) | ||
elif isinstance(elem.element, core.Variable): | ||
|
@@ -383,6 +387,8 @@ def _write_variable_declaration(self, elem: core.Variable) -> None: | |
self._write("extern ") | ||
if isinstance(elem.data_type, core.Type): | ||
self._write_type_declaration(elem.data_type) | ||
elif isinstance(elem.data_type, core.Enum): | ||
self._write_enum_usage(elem.data_type) | ||
elif isinstance(elem.data_type, core.Struct): | ||
self._write_struct_usage(elem.data_type) | ||
elif isinstance(elem.data_type, core.Declaration): | ||
|
@@ -432,7 +438,7 @@ def _write_typedef_usage(self, elem: core.TypeDef): | |
""" | ||
Writes typedef usage | ||
""" | ||
if not elem.name: | ||
if elem.name == "": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is making it worse. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh ok i didn't know this one. |
||
raise ValueError("Typedef without name detected") | ||
self._write(elem.name) | ||
|
||
|
@@ -446,6 +452,8 @@ def _write_typedef_declaration(self, elem: core.TypeDef): | |
self._write("const ") | ||
if isinstance(elem.base_type, core.Type): | ||
self._write_type_declaration(elem.base_type) | ||
elif isinstance(elem.base_type, core.Enum): | ||
self._write_enum_usage(elem.base_type) | ||
elif isinstance(elem.base_type, core.Struct): | ||
self._write_struct_usage(elem.base_type) | ||
elif isinstance(elem.base_type, core.Declaration): | ||
|
@@ -493,7 +501,7 @@ def _write_function_usage(self, elem: core.Function) -> None: | |
""" | ||
Writes function usage (name of the function) | ||
""" | ||
if not elem.name: | ||
if elem.name == "": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't change this. It's making it worse. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK |
||
raise ValueError("Function with no name detected") | ||
self._write(elem.name) | ||
|
||
|
@@ -507,6 +515,10 @@ def _write_function_declaration(self, elem: core.Function) -> None: | |
self._write("static ") | ||
if isinstance(elem.return_type, core.Type): | ||
self._write_type_declaration(elem.return_type) | ||
if isinstance(elem.return_type, core.TypeDef): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is supposed to be here without having a unit test that proves it. Nevertheless it should say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can chage it to elif. But should work similary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am doing something like this. Maybe it is not optional. Byt i would like to have function which will return STRING from declaration instead of this hack for cfile.Writer(cfile.StyleOptions()).write_str:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok this change is not necessary as i should use |
||
self._write_typedef_usage(elem.return_type) | ||
elif isinstance(elem.return_type, core.Enum): | ||
self._write_enum_usage(elem.return_type) | ||
elif isinstance(elem.return_type, core.Struct): | ||
self._write_struct_usage(elem.return_type) | ||
else: | ||
|
@@ -599,19 +611,60 @@ def _write_func_call(self, elem: core.FunctionCall) -> None: | |
self._write_expression(arg) | ||
self._write(")") | ||
|
||
def _write_enum_usage(self, elem: core.Enum) -> None: | ||
""" | ||
Writes enum usage | ||
""" | ||
if elem.name == "": | ||
raise ValueError("enum doesn't have a name. Did you mean to use a declaration?") | ||
self._write(f"enum {elem.name}") | ||
|
||
def _write_enum_declaration(self, elem: core.Enum): | ||
""" | ||
Writes enum declaration | ||
""" | ||
self._write(f"enum{" ".join([' __attribute__(('+attribute+'))' for attribute in elem.attributes])} {elem.name}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change only works for GCC-like compilers. Needs some more consideration and design (for example how to specify compiler family). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree |
||
if self.style.brace_wrapping.after_struct: | ||
self._eol() | ||
self._start_line() | ||
self._write("{") | ||
self._eol() | ||
else: | ||
self._write(" {") | ||
self._eol() | ||
if len(elem.members): | ||
self._indent() | ||
for member in elem.members: | ||
self._start_line() | ||
self._write_enum_member(member) | ||
self._write(",") | ||
self._eol() | ||
if len(elem.members): | ||
self._dedent() | ||
self._start_line() | ||
self._write("}") | ||
self.last_element = ElementType.ENUM_DECLARATION | ||
|
||
def _write_enum_member(self, elem: core.EnumMember) -> None: | ||
""" | ||
Writes enum member | ||
""" | ||
result = elem.name + " = " + str(elem.value) | ||
self._write(result) | ||
|
||
def _write_struct_usage(self, elem: core.Struct) -> None: | ||
""" | ||
Writes struct usage | ||
""" | ||
if not elem.name: | ||
if elem.name == "": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't change this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK |
||
raise ValueError("struct doesn't have a name. Did you mean to use a declaration?") | ||
self._write(f"struct {elem.name}") | ||
|
||
def _write_struct_declaration(self, elem: core.Struct) -> None: | ||
""" | ||
Writes struct declaration | ||
""" | ||
self._write(f"struct {elem.name}") | ||
self._write(f"struct{" ".join([' __attribute__(('+attribute+'))' for attribute in elem.attributes])} {elem.name}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as before. Needs some further consideration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree |
||
if self.style.brace_wrapping.after_struct: | ||
self._eol() | ||
self._start_line() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this? It's off topic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less complicated API. Because what is difference between "" or None? You are checking for None but not for "". So it is simpler and easier to check for "" then for None and ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So based on what you said bellow i understand why you are using None, but still i think it is redundant.