Add support for enums and attributes by Hadatko · Pull Request #13 · cogu/cfile

Hadatko · 2024-10-21T12:54:04Z

Refactored structs

Refactored structs Signed-off-by: Cervenka Dusan <cervenka@acrios.com>

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>

cogu

I cannot approve this pull request as the code is not up to standard for the following reasons:

Contains unrelated and off-topic changes
Use of empty lists as default values in functions
Introduction of attributes is compiler-dependent and needs further consideration
Most importantly, it doesn't have any unit tests

If you make a new PR containing just the Enum with passing unit tests I might approve it. Other changes needs to come later.

cogu · 2024-10-22T08:44:49Z

src/cfile/core.py

    Base class for all data types
    """
-    def __init__(self, name: str | None) -> None:
+    def __init__(self, name: str = "") -> None:


Why change this? It's off topic

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 ""

So based on what you said bellow i understand why you are using None, but still i think it is redundant.

cogu · 2024-10-22T08:45:11Z

src/cfile/core.py

        else:
            raise KeyError(name)

+class EnumMember(Element):


This should be named EnumValue

Actually not. It is like StructMember. This is also member of Enum type, which has name and value

Another point of view:
If this should be EnumValue, then StructMember should be StructVariable.

How about StructElement?

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

Could be also StructItem and EnumItem

cogu · 2024-10-22T08:46:09Z

src/cfile/core.py

+    A enum definition
+    """
+
+    def __init__(self, name: str, members: list[EnumMember] = [], attributes: list[str] = []) -> None:


You must never use empty list as default value in functions. It's dangerous.

Why? It is valid case, where you have no members.

t's a very common mistake among newcomers to Python. Google it, there are great explanations on the internet.

That just sad.
Ok we can use None. But i wouldn't use one membert type as an option.
so list[EnumMember] | None = None
in a code bellow:
self.members = list(working_list) if working_list else []

It's safer to do a proper check against None.

def __init__(self, name: str, elements: list[EnumElement] | None = None): self.elements: list[EnumElement] = [] if elements is not None: self.elements = list(elements)

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 append or append_<something> that does the actual check.

cogu · 2024-10-22T08:59:40Z

src/cfile/core.py

    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:


Never use empty list as initializer.
Introducing attributes here needs some further design considerations. Attributes are compiler-specific for GCC. What if we are using MSVC? We should introduce the attribute mechanism separately.

I agree, i just need this one so i implemented them this way. But i was thinking to have something like cfiles attribute class

cogu · 2024-10-22T09:02:08Z

src/cfile/core.py

+            self.members: list[StructMember] = members.copy()
+        else:
+            raise TypeError('Invalid argument type for "elements"')
+        self.attributes = attributes.copy()


Don't use copy. If you want to create a new list which is a copy of another, write it as list(attributes).
But as stated above, we should introduce attribute-mechanism later anyway.

cogu · 2024-10-22T09:21:44Z

src/cfile/writer.py

        Writes typedef usage
        """
-        if not elem.name:
+        if elem.name == "":


This is making it worse. if not elem.name works equally well if name is an empty string or None. Comparing it to empty string is not the Python way.

Oh ok i didn't know this one.

cogu · 2024-10-22T09:22:04Z

src/cfile/writer.py

        Writes function usage (name of the function)
        """
-        if not elem.name:
+        if elem.name == "":


Don't change this. It's making it worse.

cogu · 2024-10-22T09:24:46Z

src/cfile/writer.py

+        """
+        Writes enum declaration
+        """
+        self._write(f"enum{" ".join([' __attribute__(('+attribute+'))' for attribute in elem.attributes])} {elem.name}")


This change only works for GCC-like compilers. Needs some more consideration and design (for example how to specify compiler family).

cogu · 2024-10-22T09:25:03Z

src/cfile/writer.py

        Writes struct usage
        """
-        if not elem.name:
+        if elem.name == "":


Don't change this

cogu · 2024-10-22T09:25:32Z

src/cfile/writer.py

        Writes struct declaration
        """
-        self._write(f"struct {elem.name}")
+        self._write(f"struct{" ".join([' __attribute__(('+attribute+'))' for attribute in elem.attributes])} {elem.name}")


Same as before. Needs some further consideration.

cogu · 2024-10-22T09:38:53Z

src/cfile/writer.py

            self._write("static ")
        if isinstance(elem.return_type, core.Type):
            self._write_type_declaration(elem.return_type)
+        if isinstance(elem.return_type, core.TypeDef):


Not sure if this is supposed to be here without having a unit test that proves it. Nevertheless it should say elif here and not if.

I can chage it to elif. But should work similary.
Well i need it here otherwise i cannot generate my code :D

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:

"sizeof({cfile.Writer(cfile.StyleOptions()).write_str(self.cF.sequence().append(self.paramsTypes[param["data"].type]["C"]))})"

Ok this change is not necessary as i should use self.cF.func_call("sizeof", [self.paramsTypes[param["data"].type]["C"]]) instead of what i mentioned. I am developing under pressure a bit and lack of documentation is not helping :D

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>

cogu · 2024-10-24T19:15:14Z

src/cfile/core.py

+    ELSE = 2
+
+
+class Condition(Block):


Now you're taking shortcuts. Not all if-statements are wrapped in braces.
In C you can declare statements like this:

if(condition) ++x; else ++y;

cogu · 2024-10-24T19:20:08Z

src/cfile/writer.py

+
+    def _write_condition(self, elem:core.Condition) -> None:
+        if elem.type == core.ConditionType.IF:
+            self._write(f"if ({elem.condition})")


You need to add support for new formatting styles in style.py then support it in writer.py

Space after if but before parenthesis? if( vs. if (

Space around the condition? (condition) vs. ( condition )

cogu · 2024-10-24T19:26:52Z

You're taking too many shortcuts in the design. You don't add style formatting options in the writer and don't do any unit testing.

I think you should continue working directly on your fork. I'm sure you can repurpose the code to fit your specific needs.
I should close this PR.

Hadatko · 2024-10-24T19:43:02Z

Hi @cogu , i convert PR to draft. Of course i am taking shortcuts as there are too many missing features. I think you shouldn't close PR yet. You can update your code base on this in correct way. After then i would close this branch. Otherwise i would keep it here as an inspiration for people who would miss same features...

Later i can provide better PRs divided by features. E.g. adding enums, fix blank line, ...

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>

Hadatko added 3 commits October 21, 2024 14:53

Add support for enums and attributes

80606e4

Refactored structs Signed-off-by: Cervenka Dusan <cervenka@acrios.com>

Added missing Enum calls

c8e32cc

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>

Fixed typedef usage

bd63c99

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>

cogu requested changes Oct 22, 2024

View reviewed changes

cogu reviewed Oct 22, 2024

View reviewed changes

Added very basic conditions and fixed blank

f3811d5

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>

cogu reviewed Oct 24, 2024

View reviewed changes

Hadatko marked this pull request as draft October 24, 2024 19:41

Hadatko force-pushed the feature/addEnumsMainly branch 4 times, most recently from 93d296f to 9decef9 Compare November 7, 2024 17:53

Hadatko force-pushed the feature/addEnumsMainly branch 3 times, most recently from 8da198e to 5c3c3c1 Compare November 12, 2024 13:55

Added enums working in declaration assignment.

0240797

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>

Hadatko force-pushed the feature/addEnumsMainly branch from 5c3c3c1 to 0240797 Compare December 15, 2024 10:03

Conversation

Hadatko commented Oct 21, 2024

Uh oh!

cogu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hadatko Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cogu commented Oct 24, 2024

Uh oh!

Hadatko Oct 22, 2024 •

edited

Loading

Hadatko commented Oct 24, 2024 •

edited

Loading