Improve _Atomic support and add more tests #431
Conversation
pycparser/c_parser.py
Outdated
| def p_atomic_specifier(self, p): | ||
| """ atomic_specifier : _ATOMIC LPAREN type_name RPAREN | ||
| """ | ||
| p[0] = c_ast.TypeDecl( |
There was a problem hiding this comment.
@eliben is this the way you believe it should be done? Currently I am not sure whether it need to be Decl or TypeDecl. The inline docs are a bit not uneasy to follow.
There was a problem hiding this comment.
I don't think you should be creating a new TypeDecl here. Instead you should be attaching a ['atomic'] qualifier on an existing declaration.
There was a problem hiding this comment.
Hmm, but do I have one? Or do you refer to type_name?
There was a problem hiding this comment.
With the latest change it correctly generates int * _Atomic a; from _Atomic(int *) a;, but their corresponding ASTs are normalised differently:
ASTs
FileAST(ext=[Decl(name='a',
quals=[
],
storage=[
],
funcspec=[
],
type=PtrDecl(quals=['_Atomic'
],
type=TypeDecl(declname='a',
quals=[
],
type=IdentifierType(names=['int'
]
)
)
),
init=None,
bitsize=None
)
]
)
FileAST(ext=[Decl(name='a',
quals=[
],
storage=[
],
funcspec=[
],
type=TypeDecl(declname='a',
quals=[
],
type=Typename(name=None,
quals=[
],
type=PtrDecl(quals=['_Atomic'
],
type=TypeDecl(declname=None,
quals=[
],
type=IdentifierType(names=['int'
]
)
)
)
)
),
init=None,
bitsize=None
)
]
)
I think the generated tree for int * _Atomic a; is correct, while _Atomic(int *) a; has some trash nodes that need to be removed. I thought this is what _build_declarations should fix, but actually it does not.
- Should these ASTs match in this form (i.e. when just
printing? - Is there any centralised code that drops such empty nodes from the AST? If not, do I need to do it locally in some particular place?
- Should the comparison code in the test be fixed instead?
There was a problem hiding this comment.
An alternative would be to avoid AST comparison at all, checking only the resulting nodes for known configurations. I need your decision here as unlike with the Atomic node type there is no simple way to provide generation code.
There was a problem hiding this comment.
Sorry, I've been busy with other things.
Whenever I look at this, it's clear that it requires more thought than a cursory review. These parts of the parser are the trickiest. I'll need to find a significant chunk of time to look into this and upload all the required state back into my head.
There was a problem hiding this comment.
Sure. For the time being we have a workaround with the node. As long as you could spare some time eventually and provide some guidance, I believe we should be able to figure it out. Currently I simply do not have enough architectural context to provide a reasonable solution.
pycparser/c_parser.py
Outdated
| @@ -783,6 +783,12 @@ def p_declaration_specifiers_no_type_3(self, p): | |||
| """ | |||
| p[0] = self._add_declaration_specifier(p[2], p[1], 'function') | |||
|
|
|||
| # This is a bit ugly, but we need to process atomic specifier before qualifiers, | |||
There was a problem hiding this comment.
Not sure what "a bit ugly" refers to here. Can you clarify the comment?
There was a problem hiding this comment.
This group of functions has no_type in the list, but this code is exactly adding type.
There was a problem hiding this comment.
Maybe it should actually be qual here.
pycparser/c_parser.py
Outdated
| def p_atomic_specifier(self, p): | ||
| """ atomic_specifier : _ATOMIC LPAREN type_name RPAREN | ||
| """ | ||
| p[0] = c_ast.TypeDecl( |
There was a problem hiding this comment.
I don't think you should be creating a new TypeDecl here. Instead you should be attaching a ['atomic'] qualifier on an existing declaration.
tests/test_c_generator.py
Outdated
| self._assert_ctoc_correct('_Atomic(int *) a;') | ||
| self._assert_ctoc_correct('_Atomic(int) *a;') | ||
| self._assert_ctoc_correct('_Atomic int *a;') | ||
| self._assert_ctoc_correct('auto const _Atomic(int *) a;') |
There was a problem hiding this comment.
I rebased the PR on top of the changes and provided a fix to typedefs, yet there still is broken stuff apparently. E.g. this line auto const _Atomic(int *) a; generates auto int * _Atomic a; losing const.
There was a problem hiding this comment.
The issue is that CGenerator does not print quals for Decl. I.e. the following AST is generated:
auto const _Atomic(int *) a;
FileAST(ext=[Decl(name='a',
quals=['const'
],
storage=['auto'
],
funcspec=[
],
type=PtrDecl(quals=['_Atomic'
],
type=TypeDecl(declname='a',
quals=[
],
type=IdentifierType(names=['int'
]
)
)
),
init=None,
bitsize=None
)
]
)
and then the generator turns it into:
auto int * _Atomic a;
FileAST(ext=[Decl(name='a',
quals=[
],
storage=['auto'
],
funcspec=[
],
type=PtrDecl(quals=['_Atomic'
],
type=TypeDecl(declname='a',
quals=[
],
type=IdentifierType(names=['int'
]
)
)
),
init=None,
bitsize=None
)
]
)
An obvious fix is to add quals printing in def _generate_decl(self, n):.
diff --git a/pycparser/c_generator.py b/pycparser/c_generator.py
index ded8c65..5a977e5 100644
--- a/pycparser/c_generator.py
+++ b/pycparser/c_generator.py
@@ -418,6 +418,7 @@ class CGenerator(object):
s = ''
if n.funcspec: s = ' '.join(n.funcspec) + ' '
if n.storage: s += ' '.join(n.storage) + ' '
+ if n.quals: s += ' '.join(n.quals) + ' '
s += self._generate_type(n.type)
return sBut that route is a bit complicated as it leads to duplicating quals, e.g. _Atomic int x; gives _Atomic _Atomic int x; now. This feels wrong in the source, because I see the _Atomic qualifier being present twice in the AST:
FileAST(ext=[Decl(name='x',
quals=['_Atomic'
],
storage=[
],
funcspec=[
],
type=TypeDecl(declname='x',
quals=['_Atomic'
],
type=IdentifierType(names=['int'
]
)
),
init=None,
bitsize=None
)
]
)
Was the original intention to duplicate quals in both Decl and TypeDecl but use Decl for user interaction only? This feels wrong, as I cannot put const in the first example to e.g. PtrDecl quals, as it would then refer to the inner type.
There was a problem hiding this comment.
I faced a very similar issue with _Alignas(x) earlier, where I initially put _Alignas into quals. To overcome this I added a new align attribute here: https://github.com/eliben/pycparser/pull/428/files#diff-1d8b642cdf67bf7baea048a41f5397095407a6cf7768cd9af7356d259d710debR427
There was a problem hiding this comment.
I'm OK with leaving the failing c-generator tests out (or commenting them out with explanation) for now and landing this. The generator is interesting but it's by far the lowest priority capability of pycparser.
Please move the additional testing (like typedef) into test_c_parser. In general this file is better for detailed parsing tests.
|
Fits me. Pushed. |
vit9696
changed the title
closes #430