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: Review of high prio headers from engines #2633

Merged
merged 1 commit into from Dec 2, 2020

Conversation

@b-gent
Copy link
Contributor

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

This is a doxygen review for 4 header files that have been
identified as high priority for GSoD:

  • advancedDetector.h
  • metaengine.h
  • engine.h
  • util.h

It also adds doxy groups to other headers from this folder to
make them appear properly in the structure.

HTML output available here:
https://b-gent.github.io/2633/de/d3d/group__engines__advdetector.html

@sev- @criezy

@b-gent
Copy link
Contributor Author

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

I wasn't sure what to do with file obsolete.h , should I add doxygen groups to it? Its name suggests maybe we should exclude it from the doc build?

@criezy
Copy link
Member

@criezy criezy commented Nov 22, 2020

I wasn't sure what to do with file obsolete.h , should I add doxygen groups to it? Its name suggests maybe we should exclude it from the doc build?

I didn't even know that this file existed. After more than 10 years I am still discovering some new aspect of the code base :-)

A quick look indicates that the code in this file is not itself obsolete. It is to deal with obsolete game IDs (when we change the ID of a game between ScummVM versions) and handles update of the config file. So it should not be excluded from the doc.

Copy link
Member

@criezy criezy left a comment

I have reviewed all the files except engine.h (this is a big one, and I am keeping it for tomorrow).

Overall this looks good, but there are a few mistakes.

engines/advancedDetector.h Outdated Show resolved Hide resolved
engines/metaengine.h Outdated Show resolved Hide resolved
engines/metaengine.h Outdated Show resolved Hide resolved
engines/metaengine.h Outdated Show resolved Hide resolved
engines/metaengine.h Outdated Show resolved Hide resolved
engines/metaengine.h Outdated Show resolved Hide resolved
Copy link
Member

@criezy criezy left a comment

I finished reviewing this pull request, and there are a few more changes that needs to be made before it can be merged.

engines/engine.h Outdated Show resolved Hide resolved
engines/engine.h Outdated Show resolved Hide resolved
engines/engine.h Outdated Show resolved Hide resolved
engines/engine.h Outdated Show resolved Hide resolved
engines/engine.h Outdated Show resolved Hide resolved
engines/engine.h Show resolved Hide resolved
@b-gent b-gent force-pushed the b-gent:doxygen_review_engines branch 2 times, most recently from f5c681b to 1819376 Nov 30, 2020
@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Dec 1, 2020

Comments fixed, rebased, and conflicts resolved.

Should be good to merge @criezy

@b-gent b-gent force-pushed the b-gent:doxygen_review_engines branch 2 times, most recently from dffcf04 to fc0b38d Dec 1, 2020
@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Dec 1, 2020

Rebased.

This is a doxygen review for 4 header files that have been
identified as high priority for GSoD:

- advancedDetector.h
- metaengine.h
- engine.h
- util.h

It also adds doxy groups to other headers from this folder to
make them appear properly in the structure.
@b-gent b-gent force-pushed the b-gent:doxygen_review_engines branch from fc0b38d to d607e39 Dec 2, 2020
@b-gent
Copy link
Contributor Author

@b-gent b-gent commented Dec 2, 2020

One more amend, I realized I didn't add a doxy group to obsolete.h.
Should be all fine now.

@criezy
criezy approved these changes Dec 2, 2020
Copy link
Member

@criezy criezy left a comment

Indeed, it looks good now.
Thank you!

@criezy criezy merged commit f040565 into scummvm:master Dec 2, 2020
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
deepcode-ci-bot Well done, no issues found!
Details
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

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