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

Fix sizeof. Also inlines many sizes variable sizes directly. However,… #79

Closed
wants to merge 2 commits into from

Conversation

lerno
Copy link
Collaborator

@lerno lerno commented Dec 1, 2018

… structs are skipped as exact sizes are unclear at this point of time. A bug was found, where – for example – "Foo a = Foo;" is allowed. This requires some extra code to distinguish between typevalues and types of expressions. Since this bug already existed, I did not want to do the extra work in this commit, however, I've updated the expected error.

@bvdberg
Copy link
Member

bvdberg commented Dec 2, 2018

I see that the Parser now indeed uses the Capital case/not to determine it it's a type. There's one corner case
here (just impacts other stuff) and that's external C libraries. External C libraries are allowed to have types with lower cased characters. So that's very nasty.

I also ran into this issue with the stat() function in libC. There a function stat() and a struct stat. Hmmm..
What I still have to try is to change struct stat -> struct Stat and use an attribute (c-name='stat'). That way
we can maintain the spelling and keep this code.

Why did the following test change: test/Functions/struct_functions/static_member.c2
The error seemed ok, but now it's 'i32 cannot be assigned to value'

@lerno
Copy link
Collaborator Author

lerno commented Dec 2, 2018 via email

@bvdberg
Copy link
Member

bvdberg commented Dec 2, 2018

There are lots of call (most of them) that just fit in. printf, open/close, etc.
I wouldn't want to change all those to _c_printf, _c_open etc. For the few
that do have issues (functions only stat that i have found so far). I just did
a test with adding an attribute cname. That works, but for stat there is another
issue that I have to solve: we always generate a typedef for structs. That is not
usable for stat. I have to think when/not to generate a typedef..

@lerno
Copy link
Collaborator Author

lerno commented Dec 2, 2018

@bvdberg re-read my suggestion.

printf would be available as is without change. The prefix is only for the ones not matching our conventions.

@bvdberg bvdberg added this to the 1 milestone Dec 2, 2018
@lerno
Copy link
Collaborator Author

lerno commented Dec 5, 2018

Any other questions on this one @bvdberg ? Next after this is tackling the struct packing / calculations

… structs are skipped as exact sizes are unclear at this point of time. A bug was found, where – for example – "Foo a = Foo;" is allowed. This requires some extra code to distinguish between typevalues and types of expressions. Since this bug already existed, I did not want to do the extra work in this commit, however, I've updated the expected error.
@lerno
Copy link
Collaborator Author

lerno commented Dec 8, 2018

I removed one diff in indent to make the diff even smaller. Will there be time to merge this?

@bvdberg
Copy link
Member

bvdberg commented Dec 10, 2018

Could you please add some tests to show what C-code is generated for the new generator?

What I see is that i32 a = sizeof(i32); turns into (C-code) int32_t a = sizeof(4);
we could also do that later, since it's already a step in the right direction howevery.

@lerno
Copy link
Collaborator Author

lerno commented Dec 10, 2018 via email

@lerno
Copy link
Collaborator Author

lerno commented Dec 10, 2018

I realized you're right, the C-gen was not there. Added now.

@bvdberg
Copy link
Member

bvdberg commented Dec 14, 2018

merged after adding some tests and fixing some static struct stuff..

@bvdberg bvdberg closed this Dec 14, 2018
@lerno
Copy link
Collaborator Author

lerno commented Dec 14, 2018

Would you mind explaining what you did @bvdberg? Looks like you changed a lot compared to my implementation.

@bvdberg
Copy link
Member

bvdberg commented Dec 16, 2018

I did some refactoring indeed. The base was to get the unit test test/Expr/member_expr.c2 running correctly. Most changes have nothing to do with @lerno 's patch, but issues already present.

  • We did not keep track of whether a function was a static-struct func or not. So some code moved from FunctionAnalyser to FileAnalyser (like isStaticStructFunc() and it's utility function getStructDecl)
  • To fix the member_expr unit test, the analyser has to keep track of whether or not to allow static access to member. For sizeof(x), it's allowed. Static struct functions are also allowed. But other static use of nonstatic struct members is not. The variable allowStaticMember is used for this.
  • The analyseStructMember broke some unit-tests by allowing too much. This function was refactored, based on the unit tests.
  • added some diagnostic messages for accessing non-static members
  • added unit tests

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