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.

Sign up for GitHub

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

Jump to bottom

Patch 1 #11

Merged
merged 5 commits into from
Jun 12, 2023
Merged

Patch 1 #11

merged 5 commits into from
Jun 12, 2023

Conversation

mr-cheff
Copy link

No description provided.

@mr-cheff
Copy link
Author

  1. made the header file a bit better
  2. msprint can now print to other streams, not just stdout (default: stdout).

@Khhs167
Copy link
Owner

Khhs167 commented Jun 12, 2023

Honestly didn't see this til' now, sorry about that!

I like the idea, but I personally have a couple issues with it:

  • Macro magic. Instead of defining some messy functions and making a macro for msprint, I'd personally reccomend having a msprint and msfprint or something in that direction.
  • stdbool. It is included once and never used, remove that for me will ya?

@mr-cheff
Copy link
Author

I added stdbool because I noticed you defined TRUE and FALSE macros. I will change the macro hell into different functions 👍🏽

@mr-cheff
Copy link
Author

Please notice that the docstrings you included in the source file is not actually being detected by any sort of documentation generator. I would recomend moving those comments to the header file.

Khhs167
Khhs167 requested changes Jun 12, 2023
View reviewed changes
Copy link
Owner

@Khhs167 Khhs167 left a comment

Choose a reason for hiding this comment

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

A couple things that I pointed out, but otherwise this is looking great!

examples/msrollback.c Outdated Show resolved Hide resolved
examples/msrollback.c Outdated Show resolved Hide resolved
include/memstack.h Outdated Show resolved Hide resolved
@mr-cheff mr-cheff requested a review from Khhs167 June 12, 2023 19:55
Khhs167
Khhs167 approved these changes Jun 12, 2023
View reviewed changes
Copy link
Owner

@Khhs167 Khhs167 left a comment

Choose a reason for hiding this comment

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

This looks fine by me.

@Khhs167 Khhs167 merged commit ac10c58 into Khhs167:master Jun 12, 2023
@mr-cheff mr-cheff deleted the patch-1 branch June 12, 2023 19:58
@mr-cheff
Copy link
Author

thank-you

@Khhs167 Khhs167 mentioned this pull request Jun 13, 2023
Closed
@Khhs167 Khhs167 added this to the Next Release(1.3.0/1.2.1) milestone Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Khhs167 Khhs167 Khhs167 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Next Release(1.3.0/1.2.1)
Development

Successfully merging this pull request may close these issues.
2 participants
@mr-cheff @Khhs167