Implement _Alignas and _Alignof support with tests #435

vit9696 · 2021-09-20T16:58:30Z

Rebased the original PR on master.

pycparser/c_lexer.py

eliben · 2021-09-21T14:21:45Z

pycparser/_c_ast.cfg


 # A typedef declaration.
 # Very similar to Decl, but without some attributes
 #
-Typedef: [name, quals, storage, type*]
+Typedef: [name, quals, align, storage, type*]


Can typedefs have _Alignas?

eliben · 2021-09-21T14:24:21Z

pycparser/_ast_gen.py

@@ -134,6 +134,22 @@ def _gen_children(self):

        return src

+    def _gen_eq(self):


Is this related to the PR?

Yes, required by tests.

Could you please clarify which tests need this? pycparser has hundreds of AST tests which don't need this comparison, so I'm wondering what's new here. In any case I would prefer to decouple this large change from the _Align* PR.

This one. I am fine to create a separate PR for that.

====================================================================== FAIL: test_alignment (tests.test_c_generator.TestCtoC) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/user/AppleDeveloper/pycparser/tests/test_c_generator.py", line 100, in test_alignment self._assert_ctoc_correct('_Alignas(32) int b;') File "/Users/user/AppleDeveloper/pycparser/tests/test_c_generator.py", line 79, in _assert_ctoc_correct "{!r} != {!r}".format(src, src2)) AssertionError: '_Alignas(32) int b;' != '_Alignas(32) int b;\n'

Alright, let's move the equality stuff out to a separate PR that can be discussed after this one lands.

eliben · 2021-09-21T14:26:16Z

pycparser/_ast_gen.py

@@ -134,6 +134,22 @@ def _gen_children(self):

        return src

+    def _gen_eq(self):
+        src  = '    def __eq__(self, o):\n'
+        src += '        if not o: return False\n'


Is this comparing children?

Which particular case do you mean?

if self.{0} != o.{0} will trigger recursive __ne__ → __eq__ calls. If you mean inner members.

eliben · 2021-09-25T13:21:18Z

pycparser/_ast_gen.py

@@ -134,6 +134,22 @@ def _gen_children(self):

        return src

+    def _gen_eq(self):


Could you please clarify which tests need this? pycparser has hundreds of AST tests which don't need this comparison, so I'm wondering what's new here. In any case I would prefer to decouple this large change from the _Align* PR.

eliben · 2021-09-25T13:34:49Z

tests/test_c_parser.py

@@ -618,6 +623,12 @@ def test_sizeof(self):
                        ['TypeDecl',
                            ['IdentifierType', ['int']]]]]])

+    def test_alignof(self):


I'd like to see more parser tests here.

At least one test separately for _Alignof without _Alignas (look at the existing sizeof tests)
A test for _Alignas with a true constant like 4
A test for _Alignas with a type inside e.g. _Alignas(int).

Done, thanks.

eliben · 2021-09-25T13:39:07Z

pycparser/c_parser.py

@@ -310,6 +310,11 @@ def _fix_decl_name_type(self, decl, typename):
        decl.name = type.declname
        type.quals = decl.quals[:]

+        for tn in typename:
+            if isinstance(tn, c_ast.Alignas):
+                type.align = tn


I'm a bit confused here: isn't align an attribute, not a child node? Should it be a child node instead if you're assigning nodes to it?

It is an attribute indeed, not a child node. This code was merely a fixup for the parser limitation. Previously one of the tests triggered an issue without this code, however, currently none does. I am in a good faith to believe that the issue was resolved more properly in one of the future iterations of the code and I did not notice it. Will simply remove.

eliben · 2021-09-25T13:39:33Z

pycparser/c_parser.py

+        for tn in typename:
+            if isinstance(tn, c_ast.Alignas):
+                type.align = tn
+                typename.remove(tn)


Don't modify a list you're iterating.

Also, if we allow a list of alignment specifiers, does it show here or do we only overwrite them?

eliben · 2021-09-25T13:44:43Z

pycparser/c_parser.py

@@ -920,9 +940,20 @@ def p_specifier_qualifier_list_3(self, p):
    def p_specifier_qualifier_list_4(self, p):
        """ specifier_qualifier_list  : type_qualifier_list type_specifier
        """
-        spec = dict(qual=p[1], storage=[], type=[], function=[])
+        spec = dict(qual=p[1], alignment=[], storage=[], type=[], function=[])


So you really need _add_declaration_specifier here? Can't you just copy the type?

Rebased on top of master fixing. Indeed no need.

eliben · 2021-09-25T13:45:22Z

pycparser/c_parser.py

+        """ specifier_qualifier_list  : alignment_specifier
+        """
+        spec = dict(qual=[], alignment=[], storage=[], type=[], function=[])
+        p[0] = self._add_declaration_specifier(spec, p[1], 'alignment')


Same question here, it's not clear why you need to create an empty spec and pass it to _add_declaration_specifier

Sounds like literal copy-pasting from the existing code, nice catch. Also fixed.

eliben

Thanks, this looks close just. Just remove the equality generation for now, and I'll take a final look

eliben · 2021-10-04T12:55:49Z

pycparser/_ast_gen.py

@@ -134,6 +134,22 @@ def _gen_children(self):

        return src

+    def _gen_eq(self):


Alright, let's move the equality stuff out to a separate PR that can be discussed after this one lands.

vit9696 · 2021-10-04T13:00:16Z

Sure, shall I comment the test part for now?

eliben · 2021-10-04T13:06:26Z

Sure, shall I comment the test part for now?

No, please remove everything related to generating equality and the test that requires it from the PR. I'm still not convinced this is needed at all, and wanted to separate it to its own PR so as not to block the align feature

vit9696 · 2021-10-04T13:10:44Z

No, please remove everything related to generating equality and the test that requires it from the PR. I'm still not convinced this is needed at all, and wanted to separate it to its own PR so as not to block the align feature

Hmm, this is the whole def test_alignment function though. There is no single test step that passes without the __eq__ functions. Updated the PR.

vit9696 · 2021-10-04T13:11:15Z

tests/test_c_generator.py

@@ -96,6 +96,27 @@ def test_complex_decls(self):
        self._assert_ctoc_correct('int test(const char* const* arg);')
        self._assert_ctoc_correct('int test(const char** const arg);')

+    # FIXME: These require custom equality comparison.


I can remove that block if you think it is better.

…ests This lets us re-enable that were commented out in #435

eliben · 2021-10-04T13:58:55Z

I've re-enabled the commented-out tests in d4c2922

vit9696 · 2021-10-04T14:32:12Z

Thanks a lot!

eliben · 2021-10-04T16:32:37Z

Thank you for pushing the align and other changes through and improving C11 support for pycparser :)

eliben reviewed Sep 21, 2021

View reviewed changes

eliben requested changes Sep 25, 2021

View reviewed changes

vit9696 added 5 commits October 2, 2021 03:43

Implement _Alignas and _Alignof support with tests

f2c3d6d

Improve testing and avoid unnecessary alignas for typedef

349a9b3

Add more tests

de194bf

Drop legacy artifact

2a42975

Remove extra _add_declaration_specifier call

c2947fc

vit9696 requested a review from eliben October 2, 2021 00:46

eliben requested changes Oct 4, 2021

View reviewed changes

Drop custom equality comparators for now

9bd74cb

vit9696 commented Oct 4, 2021

View reviewed changes

eliben approved these changes Oct 4, 2021

View reviewed changes

eliben merged commit 14ffe7b into eliben:master Oct 4, 2021

eliben added a commit that referenced this pull request Oct 4, 2021

Improve recursive comparison of ASTs in test_c_generator and enable t…

d4c2922

…ests This lets us re-enable that were commented out in #435

Munken mentioned this pull request Jan 23, 2022

pycparser 2.21 breaks generation vberlier/narmock#25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement _Alignas and _Alignof support with tests #435

Implement _Alignas and _Alignof support with tests #435

vit9696 commented Sep 20, 2021

eliben Sep 21, 2021

vit9696 Sep 21, 2021

eliben Sep 21, 2021

vit9696 Sep 21, 2021

eliben Sep 25, 2021

vit9696 Oct 2, 2021

eliben Oct 4, 2021

eliben Sep 21, 2021

vit9696 Sep 21, 2021

vit9696 Sep 21, 2021

eliben Sep 25, 2021

eliben Sep 25, 2021

vit9696 Oct 2, 2021

eliben Sep 25, 2021

vit9696 Oct 2, 2021

eliben Sep 25, 2021

eliben Sep 25, 2021

vit9696 Oct 2, 2021

eliben Sep 25, 2021

vit9696 Oct 2, 2021

eliben Sep 25, 2021

vit9696 Oct 2, 2021

eliben left a comment

eliben Oct 4, 2021

vit9696 commented Oct 4, 2021

eliben commented Oct 4, 2021

vit9696 commented Oct 4, 2021

vit9696 Oct 4, 2021

eliben commented Oct 4, 2021

vit9696 commented Oct 4, 2021

eliben commented Oct 4, 2021

		@@ -134,6 +134,22 @@ def _gen_children(self):

		return src

		def _gen_eq(self):

Implement _Alignas and _Alignof support with tests #435

Implement _Alignas and _Alignof support with tests #435

Conversation

vit9696 commented Sep 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vit9696 commented Oct 4, 2021

eliben commented Oct 4, 2021

vit9696 commented Oct 4, 2021

Choose a reason for hiding this comment

eliben commented Oct 4, 2021

vit9696 commented Oct 4, 2021

eliben commented Oct 4, 2021