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

#252 - groups the public and private class members. #111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tempelmann
Copy link
Contributor

Makes the .h file neat and tidy :)

@tempelmann
Copy link
Contributor Author

Note that I "abused" the ImportList for collecting the lines. I didn't want to create yet another class with the same functionality.

@tempelmann
Copy link
Contributor Author

About the bot warnings: I should probably declare them private.

Also: It's still buggy: Comments (from "doc:") do not generate correctly.

@tempelmann
Copy link
Contributor Author

The comments bug comes from the fact that I only add unique lines - a mistake, when it comes to comment lines. Fixing that now...

@tempelmann
Copy link
Contributor Author

tempelmann commented Sep 2, 2017

Alright. Fixed. I ended up not using the ImportList (instead using my own StringList class) and added a stack to properly manage the collector objects.

@kaitai-io kaitai-io deleted a comment Sep 2, 2017
@kaitai-io kaitai-io deleted a comment Sep 2, 2017
@tempelmann
Copy link
Contributor Author

I can't avoid the mutable warning, though. I have to change the publicDecls between classHeader and classFooter, to handle nesting of classes. No idea how to do that otherwise.

@GreyCat
Copy link
Member

GreyCat commented Sep 13, 2017

I've reviewed this PR and I believe there are quite a few problems with it:

  • It changes a lot of things not related to the topic and not mentioned anywhere in these commit messages and/or PR text, for example:
  • It adds a new private class StringList, which seems to be completely redundant for me — one can just use ListBuffer[String] instead of it.
  • Two instances of this class are wrapped into Stack for some unknown reason
  • Whole ensureMode system is heavily changed to use stuff like privateDecls.addUnique instead. It's not idempotent replacement, and, besides, addUnique is useless there anyway — these calls generated syntactically correct (i.e. unique) declarations before, so after a change they should be unique anyway.

It its current state it's unlikely to be accepted. I'd suggest that we rework it using existing LanguageOutputWriter buffering classes: we have outHdr, outSrc, etc, now, so it's quite easy to add outHdrPrivate and outHdrPublic, and dump it to main outHdr — as you do — in a class footer.

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