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

Added annotations to individual fields in a struct #324 #346

Closed
wants to merge 8 commits into from

Conversation

aiverson
Copy link
Contributor

Initial implementation of #324
Multiple comma separated annotations are not yet supported.

@aiverson
Copy link
Contributor Author

Does anyone know what is broken on AppVeyor here?

@elliottslaughter
Copy link
Member

I'm not sure why AppVeyor is failing, but I'd recommend looking at the changes to genclangpaths.lua/geninternalizedfiles.lua since those are involved in generating header files that will be imported into C code. (By the way, why are these changes required?) I suppose it could also be in lparser.cpp, but I'm having a hard time seeing how that change is related. In theory, it should be impossible for changes to the other Lua/Terra files to result in compile errors like this.

@aiverson
Copy link
Contributor Author

aiverson commented Feb 12, 2019

The change in geninternalizedfile.lua makes it not syntax error when it doesn't have any lua files to internalize.

The change in genclangpaths.lua makes it so that it can compile when the clang path has a space in it, for instance "C:\Program Files\Clang\clang.exe"

I encountered both of these problems while trying to build from source on windows, but eventually gave up on trying to get my windows system configured to build it.

Neither of them are necessary for the change I implemented, but they are improvements to the stability of the build process, I think.

@elliottslaughter
Copy link
Member

Maybe we should split those into a different PR? They seem like reasonable changes, but for this specific PR it'll be easier to debug if there's less to look at.

@aiverson
Copy link
Contributor Author

Sure, I'll revert that commit and try again.

@aiverson
Copy link
Contributor Author

That didn't magically fix it.

@elliottslaughter
Copy link
Member

Oh, please set the PR to be based on master. develop is in a strange state and I have no idea if it builds at all in AppVeyor.

@aiverson
Copy link
Contributor Author

Oh. That could be the problem. I'll put it on master.

@aiverson
Copy link
Contributor Author

Is there any chance of putting it in a good state soon?

@elliottslaughter
Copy link
Member

I don't think we're ever going to merge develop in its entirety. The parts that have been useful we've been pulling piecemeal into master. Other parts we've taken a fairly different approach on (PUC Lua support) which results in substantially less churn vs what had been done on develop. There are still some parts that would be nice to have (LLVM 3.8 debug symbols), but I haven't put in the effort to extract them.

@elliottslaughter
Copy link
Member

Closing since this is superseded by #347 (if I understand correctly---please let me know if I'm misunderstanding).

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