Skip to content

Add support for attribute in between pointer and declaration name - #59

Closed
DCNick3 wants to merge 3 commits into
inducer:mainfrom
DCNick3:fptr-improvements
Closed

DCNick3 wants to merge 3 commits into
inducer:mainfrom
DCNick3:fptr-improvements

Conversation

@DCNick3

@DCNick3 DCNick3 commented May 29, 2021

Copy link
Copy Markdown
  • Add support for declarations like int (* __attribute__((stdcall)) fptr)(void)
  • Add a test case for the aforementioned case
  • Fix regression introduced in Added support for Asm labels #54, which caused function pointer information to be discarded in case any attributes or asm labels were present

- Add support for declarations like `int (* __attribute__((stdcall)) fptr)(void)`
- Fix regression introduced in inducer#54, which caused function pointer information to be discarded in case any attributes or asm labels were present
Comment thread test/test_pycparserext.py Outdated
Comment on lines +310 to +316
from pycparserext.ext_c_parser import GnuCParser
p = GnuCParser()
ast = p.parse(src)
ast.show()

from pycparserext.ext_c_generator import GnuCGenerator
print(GnuCGenerator().visit(ast))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this tests that the parser and generator digest these, I would prefer a round-trip test asserting that the same code is generated again.

@DCNick3

DCNick3 commented May 29, 2021

Copy link
Copy Markdown
Author

While trying to come up with more test cases for the round-trip tests I found myself staring at what generator produced for this code:

int __attribute__((stdcall)) func1(void);
int func2(void) __attribute__((stdcall));

generated code:

 __attribute__((stdcall)) int func1(void);
int func2(void) __attribute__((stdcall));

I was puzzled why the attributes were located at different places in the generated code, while I thought they were put in the same place. It appears that they do not, with the former line putting the attributes into the AttributeSpecifier and it to the funcspec of the Decl and the latter - to the attributes of FuncDeclExt. Not clear to me if the distinction between those two cases should be made (pretty sure gcc does not care when compiling).

Will probably brush up this PR and dive a little bit deeper into the topic to better understand what is happening and how it should happen, probably coming with more PRs.

@DCNick3

DCNick3 commented May 30, 2021

Copy link
Copy Markdown
Author

Should be ready to merge. I was not able to make the _round_trip_matches work (it tried to compare two AttributeSpecifier's and failed there. Replaced with a check against a hard-coded code string.

@inducer

inducer commented May 30, 2021

Copy link
Copy Markdown
Owner

You could make an "abbreviated" _round_trip_matches (_round_trip_match_source_only?) that asserts that the parsed source gets regenerated.

Also, please fix the linter failures.

@DCNick3

DCNick3 commented May 31, 2021

Copy link
Copy Markdown
Author

the parsed source gets regenerated

Do you intend to compare the original source code to the generated? Or do you mean checking that after regenerating the code two times (src -> AST1 -> regen1 -> AST2 -> regen2) regen1 == regen2?

The former will fail because the AST does not store, for example, the exact attribute position and it will get moved.

The latter will work, but it would not catch all the cases that the current comparison does. For example, it might just discard the attribute information and it will pass the test, even though it is horribly wrong.

Comment thread test/test_pycparserext.py

# _round_trip_matches trips at comparing the function
# specifiers here, as they are lists
# (how was this supposed to work, again?)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean here? If something has broken semantics, just fix it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it has broken semantics, but I don't know how to fix it yet. It requires a bit more research into the codebase.

@inducer

inducer commented May 31, 2021

Copy link
Copy Markdown
Owner

The former will fail because the AST does not store, for example, the exact attribute position and it will get moved.

But there might be a source representation that's exactly reproduced. We could parse and regenerate that. Then, additional strings could be provided that are supposed to result in equivalent ASTs.

@DCNick3

DCNick3 commented May 31, 2021

Copy link
Copy Markdown
Author

Yeah, it is possible, but I want to be able to test that attributes are parsed correctly in all possible places, not only in their "canonical" form. That's why it's problematic..

@inducer

inducer commented May 31, 2021

Copy link
Copy Markdown
Owner

I want to be able to test that attributes are parsed correctly in all possible places, not only in their "canonical" form

I agree with that goal. That's where I was going with the proposal of additional strings that parse/regenerate to the same canonical AST.

@DCNick3

DCNick3 commented May 31, 2021

Copy link
Copy Markdown
Author

So, you want them in addition to what I already have? Probably can do that

@inducer

inducer commented May 31, 2021

Copy link
Copy Markdown
Owner

So, you want them in addition to what I already have? Probably can do that

Yeah, if the "reproduces canonical form" thing is easy to do, I'd appreciate having it. If it's not easy, don't bother though.

@DCNick3

DCNick3 commented Nov 20, 2023

Copy link
Copy Markdown
Author

I won't be pursuing this in any near time, sorry

@DCNick3 DCNick3 closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants