Skip to content
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

Redeclared types #1

Merged
merged 1 commit into from Jul 13, 2013
Merged

Redeclared types #1

merged 1 commit into from Jul 13, 2013

Conversation

Syeberman
Copy link
Contributor

While using pycparser to parse a large, existing codebase, I immediately came upon the typedef-name problem. The changes in this pull request resolve and test for the issues encountered; in particular:

  • Reusing typedef names as structure/union member names
  • Reusing typedef names as variables names in inner scopes
  • Reusing typedef names as parameter names in declarations and definitions
  • Duplicated typedef declarations (non-standard, but apparently common and syntactically
    similar to the above)

There is a corner case regarding parameter name scoping that required access to yacc.py's lookahead token (see p_direct_declarator_5 in c_parser.py for details). There are three solutions as I see it:

  • Modify yacc.py to expose the lookahead token as an attribute of the parser (...but requiring future PLY updates to be merged, not copied)
  • Keep track of the most-recent token via a custom tokenfunc (...but trusting that the parser's lookaheadstack is empty)
  • Use the inspect module to grab the value of the parser's lookahead variable (...but direct
    inspection of Python frames and local dictionaries is pretty evil)

For the purpose of this change I decided to use the inspect module, as it is the least error-prone (it can inspect lookaheadstack to ensure it's empty) and the least invasive to the codebase. I can instead make lookahead/lookaheadstack attributes, if you're willing to support the merging of PLY.

@eliben
Copy link
Owner

eliben commented Jun 12, 2013

In the description of alternatives, you've mentioned:

Keep track of the most-recent token via a custom tokenfunc (...but trusting that the parser's lookaheadstack is empty)

I'm not sure we care much about lookaheadstack here, as it seems to be only used for error recovery. Your code just asserts it anyway. Would this make the idea of using a custom tokenfunc more feasible?

@Syeberman
Copy link
Contributor Author

I'll look into it.

@eliben
Copy link
Owner

eliben commented Jun 13, 2013

In the description of alternatives, you've mentioned:

Keep track of the most-recent token via a custom tokenfunc (...but trusting that the parser's lookaheadstack is empty)

I'm not sure we care much about lookaheadstack here, as it seems to be only used for error recovery. Your code just asserts it anyway. Would this make the idea of using a custom tokenfunc more feasible?

@eliben eliben closed this Jun 13, 2013
@eliben eliben reopened this Jun 13, 2013
@Syeberman
Copy link
Contributor Author

I'll be updating the pull request shortly to use tokenfunc rather than inspect.

As for separating the struct field namespace to a separate patch: it's quite dependent on this change. "unsigned TT" looks very similar to the parser whether TT is being redeclared as a parameter, a variable, or a structure member.

@eliben
Copy link
Owner

eliben commented Jul 7, 2013

If separating struct field namespace lookups is too hard, then never mind - you can keep them combined. I'm looking forward to the new patch with tokenfunc.

@Syeberman
Copy link
Contributor Author

The pull request has been updated for tokenfunc: see 2f84c0d.

@eliben
Copy link
Owner

eliben commented Jul 9, 2013

Looks better, thanks. Can you now rebase the whole thing vs. tip so it's merge-able?

@Syeberman
Copy link
Contributor Author

It might take a few weeks to make that change, though. (Other priorities.) Is the current version sufficient for now?

@eliben
Copy link
Owner

eliben commented Jul 10, 2013

The current version with tokenfunc is fine. I just need a single commit for the merge (squash).

@Syeberman Syeberman closed this Jul 11, 2013
@Syeberman
Copy link
Contributor Author

The squashed commit should be available now.

@Syeberman Syeberman reopened this Jul 11, 2013
eliben added a commit that referenced this pull request Jul 13, 2013
Redeclared types


    Reusing typedef names as structure/union member names
    Reusing typedef names as variables names in inner scopes
    Reusing typedef names as parameter names in declarations and definitions
    Duplicated typedef declarations (non-standard, but apparently common and syntactically similar to the above)
@eliben eliben merged commit 149a05a into eliben:master Jul 13, 2013
@eliben
Copy link
Owner

eliben commented Jul 13, 2013

Merged. Thanks!

@Syeberman Syeberman deleted the redeclared-types branch July 14, 2013 11:55
jcklie pushed a commit to jcklie/ast-gen that referenced this pull request Oct 30, 2013
jcklie pushed a commit to jcklie/ast-gen that referenced this pull request Oct 30, 2013
Redeclared types


    Reusing typedef names as structure/union member names
    Reusing typedef names as variables names in inner scopes
    Reusing typedef names as parameter names in declarations and definitions
    Duplicated typedef declarations (non-standard, but apparently common and syntactically similar to the above)
@monakez monakez mentioned this pull request Jan 26, 2022
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.

None yet

2 participants