Skip to content

Fix incorrect rewritting of function with typdefed type #436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Feb 24, 2021
Merged

Conversation

john-h-kastner
Copy link
Collaborator

@john-h-kastner john-h-kastner commented Feb 18, 2021

Fixes issue #430 , an error in rewriting for function declaration such as

typedef void foo(int*);
foo bar;
void bar(int *a){};

Previously FunctionDeclBuilder only replaced the parameter declaration
of bar because nothing in the return type needed to be rewritten. This
caused the rewriting of the first declaration to be missing the return
type. With this change, FunctionDeclBuilder detects that bar is
declared using a typedef type, and generates replacement text for the
entire declaration instead of only the parameter list.


Note that this PR does not implement proper constraint generation for these functions, it only improves the rewriting logic to avoid assertion fails or generating invalid code.

Fixes a error in rewriting for function declaration such as

    typedef void foo(int*);
    foo bar;
    void bar(int *a){};

Previously `FunctionDeclBuilder` only replaced the parameter declaration
of `bar` because nothing in the return type needed to be rewritten. This
caused the rewriting of the first declaration to be missing the return
type. With this change, `FunctionDeclBuilder` detects that `bar` is
declared using a typedef type, and generates replacement text for the
entire declaration instead of only the parameter list.
Copy link
Member

@mwhicks1 mwhicks1 left a comment

Choose a reason for hiding this comment

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

LGTM. Is there a place in the code where you can put a comment about not generating the constraint variables properly, due to typedefs used for function types? Filing the issue #437 is good, but if you have a guess about where in the code it matters, you could note that.

Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

I assume a regression test is still coming?

john-h-kastner and others added 4 commits February 22, 2021 10:02
due to a macro.

Cleanup: It doesn't seem like we need the special case for typedefs in
tyToStr. Indeed, if the type is actually a TypedefType, I'd expect Clang
to print it as such.
Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

The parts I know about look good. 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.

Rewriting fails for functions with typedef-ed type
3 participants