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

Replace use of iostream-based classes #246

Closed
wants to merge 2 commits into from

Conversation

mobin-2008
Copy link
Collaborator

@mobin-2008 mobin-2008 commented Sep 16, 2023

Fixes #240

Hi.
We already discussed about some technical points about this new implementation in #240 and here's its PR

My plan

My plan has 4 sections:
low-level header: dinit-iostream.h

  • ostream
    ostream is a low-level library and will be used for writing into file descriptors, This class provide a buffer for each output stream (4K buffer size yet) and provide print() function for writing into fds. print() and printerr() are designed to replace cout and cerr. Here's basic usage of them:
dio::print("Hello World\n");
std::string service_name = "boot";
unsigned load_time = 12;
dio::printerr("Dinit loaded", service_name, "in", load_time, "seconds");
  • istream
    same as ostream but for reading from file descriptors. ToDo

high-level header: dinit-fstream.h

  • ofstream
    ofstream is a high-level library and will be used for writing into files, This class open a file and create a ostream object and will provide some functions such as open(). ToDo
  • ifstream
    same as ofstream but for reading from files. ToDo

Also dinitcheck modified to use these new header(s) for catching any regression. I hope it'll be on right track. Regards

Signed-off-by: Mobin "Hojjat" Aydinfar <[email protected]>

@mobin-2008 mobin-2008 added Enhancement/New Feature Improving things or introduce new feature A-Importance: High labels Sep 16, 2023
@mobin-2008 mobin-2008 marked this pull request as draft September 16, 2023 13:56
@@ -88,21 +88,21 @@ int main(int argc, char **argv)
service_dir_opts.set_specified_service_dir(argv[i]);
}
else {
cerr << "dinitcheck: '--services-dir' (-d) requires an argument" << endl;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to have "endl"?

Copy link
Owner

Choose a reason for hiding this comment

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

The endl there looks superfluous. It might be needed elsewhere, if the buffer needs to be flushed.

private:

cpbuffer<BUF_SIZE> buf; // Main buffer
bool stdio_sync = false; // Make sure message writed into dest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

syncing with stdio is disabled by default, Is it fine?

Copy link
Owner

Choose a reason for hiding this comment

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

is this supposed to be the equivalent of ios_base::sync_with_stdio? The comment doesn't match that. What do you mean by "sync with stdio"?

@davmac314
Copy link
Owner

Please make your own judgement and we can discuss if necessary when the PR is complete. You did ask to take on this issue so it is your responsibility! If there are questions you really can't resolve, you can ask them in the issue so we have a reasonable record of the decisions there.

I would prefer not to have draft PRs - they are distracting (for me, due to all the notifications) and not a good way to discuss technical details. If you need to show some example of code in a discussion, you can link to one of your own branches for example. I'm going to unsubscribe from this PR, you will need to notify me when it is ready.

@mobin-2008
Copy link
Collaborator Author

I will create a new PR when it mostly done

@mobin-2008 mobin-2008 closed this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Importance: High Enhancement/New Feature Improving things or introduce new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace use of iostream-based classes
2 participants