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

DOXYGEN: Changes to stream.h and system.h #2612

Conversation

@b-gent
Copy link
Contributor

@b-gent b-gent commented Nov 8, 2020

Review and improve doxygen comments in stream.h and system.h.

Current HTML output can be found here:
https://b-gent.github.io/osystem_groups/d5/d81/group__common__system__module.html

common/system.h Outdated Show resolved Hide resolved
Copy link
Member

@criezy criezy left a comment

Good work on stream.h!
There are some changes to make though (see the various comments).

I will review system.h tomorrow.

common/stream.h Outdated Show resolved Hide resolved
common/stream.h Outdated Show resolved Hide resolved
common/stream.h Show resolved Hide resolved
common/stream.h Outdated Show resolved Hide resolved
common/stream.h Outdated Show resolved Hide resolved
common/stream.h Show resolved Hide resolved
common/stream.h Outdated Show resolved Hide resolved
common/stream.h Outdated Show resolved Hide resolved
common/stream.h Show resolved Hide resolved
common/stream.h Show resolved Hide resolved
@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Nov 10, 2020

Thanks for taking this up so quickly @criezy , I'll start working on the comments during the day tomorrow.

Copy link
Member

@criezy criezy left a comment

And I have also reviewed the changes for the system.h file now.

common/system.h Outdated Show resolved Hide resolved
common/system.h Show resolved Hide resolved
common/system.h Outdated Show resolved Hide resolved
common/system.h Show resolved Hide resolved
@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Nov 10, 2020

@sev- you might want to review if you have time

@criezy
Copy link
Member

@criezy criezy commented Nov 11, 2020

One more thing: for the OSystem class, I think it would be good to have the various get*Manager() functions in the Module slots group rather than Miscellaneous.

And maybe also rename Module slots to Subsystem modules. The word slot doesn't really speak to me.

@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Nov 12, 2020

One more thing: for the OSystem class, I think it would be good to have the various get*Manager() functions in the Module slots group rather than Miscellaneous.

And maybe also rename Module slots to Subsystem modules. The word slot doesn't really speak to me.

Looks like this right now:
https://b-gent.github.io/osystem_groups/d5/d81/group__common__system__module.html

I assumed getFilesystemFactory() should also be moved there, let me know if that's correct.

@b-gent b-gent force-pushed the b-gent:improve_doxygen_in_high_priority_headers_2 branch from d3b133e to 3b39e63 Nov 12, 2020
@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Nov 12, 2020

All comments are fixed now. Would be great to merge asap to avoid conflicts @criezy

Review and improve doxygen comments in stream.h and system.h.
@b-gent b-gent force-pushed the b-gent:improve_doxygen_in_high_priority_headers_2 branch from 3b39e63 to 2dd432a Nov 12, 2020
@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Nov 12, 2020

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that.
Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Nov 12, 2020

Retry DeepCode

@criezy
criezy approved these changes Nov 12, 2020
*
* This is equivalent to calling:
* @code
* seek(offset, SEK_CUR)

This comment has been minimized.

@criezy

criezy Nov 12, 2020
Member

This should be SEEK_CUR, not SEK_CUR (sorry that was a typo in my comment).
I will still merge and then fix the typo in tree.

@criezy
Copy link
Member

@criezy criezy commented Nov 12, 2020

Merged manually to fix the conflict (there was one already!) and fix the typo on SEK_CUR.

@criezy criezy closed this Nov 12, 2020
@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Nov 12, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.