Skip to content

Fix C printer output of array types in typedefs#12

Merged
ircmaxell merged 6 commits into
ircmaxell:masterfrom
dirx:c-printer-typedef-array
May 1, 2020
Merged

Fix C printer output of array types in typedefs#12
ircmaxell merged 6 commits into
ircmaxell:masterfrom
dirx:c-printer-typedef-array

Conversation

@dirx
Copy link
Copy Markdown
Contributor

@dirx dirx commented Apr 26, 2020

#11 - WIP

Added typedef array to test case.

@dirx
Copy link
Copy Markdown
Contributor Author

dirx commented Apr 26, 2020

@ircmaxell why are non function typedefs handled differently here?

if ($decl instanceof Decl\NamedDecl\TypeDecl\TypedefNameDecl\TypedefDecl) {
if ($this->isFunction($decl->type)) {
return 'typedef ' . $this->printType($decl->type, $decl->name, $level);
}
return 'typedef ' . $this->printType($decl->type, null, $level) . ' ' . $decl->name;
}

@ircmaxell
Copy link
Copy Markdown
Owner

Honestly, I don't remember. That code always gave me lots of problems conceptually for some reason (it's re-used in a few parts), and I am not sure why any of it is how it is. I am sure that I ran into a situation that this appeared to solve, but apparently not...

* more test cases
* force * to right side of space
@dirx
Copy link
Copy Markdown
Contributor Author

dirx commented Apr 27, 2020

@ircmaxell Maybe appending names to functions did not work...

I dropped special handling & isFunction (might be a bc in case someone extended the C printer).
But: this forces * to the right side. Do you have any preference here?

I did a quick run against (a cleaned up) rdkafka.h (mhhh ... no arrays here) and uuid.h. Looks good. Running against llvm would be helpful.

@ircmaxell
Copy link
Copy Markdown
Owner

might be a bc in case someone extended the C printer

To my knowledge nobody is other than FFIMe, but can ship a major version bump if that's a big deal (I doubt it).

But: this forces * to the right side. Do you have any preference here?

It's less about preference and more about the meaning of the construct. Doesn't the position change the meaning of the declaration?

@dirx
Copy link
Copy Markdown
Contributor Author

dirx commented Apr 28, 2020

It's less about preference and more about the meaning of the construct. Doesn't the position change the meaning of the declaration?

Right. I missed the space. I meant the right side of the space:

  • int *bar > before: int * bar
  • int *foo(void *baz) > before: int * foo(void *baz)

Parentheses and arrays seem also quirky:

  • int (*waldo)[5] gives (int *waldo)[5] which gives errors on reparsing

Will take a look at this.

@ircmaxell
Copy link
Copy Markdown
Owner

Ah, as far as space, I don't really have a preference. So happy to accept whatever :)

@dirx
Copy link
Copy Markdown
Contributor Author

dirx commented Apr 29, 2020

This example helped a lot with arrays and paranthesis:

  • typedef char *(*(**hairy[][8])())[];

See http://unixwiz.net/techtips/reading-cdecl.html.

@ircmaxell could you please take if this PR works for you.

@dirx
Copy link
Copy Markdown
Contributor Author

dirx commented Apr 29, 2020

This also fixes the "function type issue" mentioned in #2.

Copy link
Copy Markdown
Owner

@ircmaxell ircmaxell left a comment

Choose a reason for hiding this comment

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

LGTM

@ircmaxell
Copy link
Copy Markdown
Owner

Are you good with this as is? If so, I'm happy to merge.

@dirx
Copy link
Copy Markdown
Contributor Author

dirx commented May 1, 2020

Stumbled over the minor void trailing space issue and fixed it.

I successfully tested binding rdkafka, snappy, uuid, portalaudio and ffi with the c printer output.
So, I am good with this.

@ircmaxell ircmaxell merged commit 812f9d4 into ircmaxell:master May 1, 2020
@ircmaxell
Copy link
Copy Markdown
Owner

Awesome, Thanks!!!

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