Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork56.4k
Fixes for various mostly trivial warnings#23109
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
alalek left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It makes sense to enable such warnings in CMake scripts to avoid regressions in the future.
| CAP_PROP_OPEN_TIMEOUT_MSEC=53,//!< (**open-only**) timeout in milliseconds for opening a video capture (applicable for FFmpeg and GStreamer back-ends only) | ||
| CAP_PROP_READ_TIMEOUT_MSEC=54,//!< (**open-only**) timeout in milliseconds for reading from a video capture (applicable for FFmpeg and GStreamer back-ends only) | ||
| CAP_PROP_STREAM_OPEN_TIME_USEC =55,//<! (read-only) time in microseconds since Jan 1 1970 when stream was opened. Applicable for FFmpeg backend only. Useful for RTSP and other live streams | ||
| CAP_PROP_STREAM_OPEN_TIME_USEC =55,///<! (read-only) time in microseconds since Jan 1 1970 when stream was opened. Applicable for FFmpeg backend only. Useful for RTSP and other live streams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
///<
In opencv code base//!< format is preferable:
$ grep -Rni '///<' ./ | wc -l207$ grep -Rni '//!<' ./ | wc -l1231(both do the same:https://www.doxygen.nl/manual/docblocks.html#memberdoc )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, indeed, I should have noticed the adjacent ones... Fixed.
seanm commentedJan 10, 2023
Yes, indeed. Though note that this PR does not fixall the warnings of these types, except for a few. I wanted to start with the trivial ones first. There are alot more -Wdocumentation warnings for example. |
| @param src2_data,src2_step second source image data and step | ||
| @param dst_data,dst_step destination image data and step | ||
| @param width,height dimensions of the images | ||
| @param src1_data first source image data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this kind of changes necessary? Doxygen supports this syntax and documentation looks fine with it:https://docs.opencv.org/4.x/d8/d6f/group__core__hal__interface__addsub.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It does not appear to be valid syntax:https://www.doxygen.nl/manual/commands.html#cmdparam
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This page says that it is allowed:
Note that you can also document multiple parameters with a single \param command using a comma separated list. Here is an example:
/** Sets the position. * @param x,y,z Coordinates of the position in 3D space. */void setPosition(double x,double y,double z,double t){}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, so it does! Strange that it is not in the actual syntax format of: \param '['dir']' <parameter-name> { parameter description }
Well, in any case, clang's -Wdocumentation does warn, I would not have changed it otherwise. I'll file a bug with clang, but since -Wdocumentaion found so many other genuine problems, I'd vote for accommodating it, especially since it's already done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ticket exists already:llvm/llvm-project#14707
asmorkalov commentedMar 21, 2023
@seanm Friendly reminder. There are merge conflicts in the PR. |
seanm commentedJun 16, 2023
@asmorkalov I lost track of this PR somehow... but merge conflicts now fixed! |
Mostly this was just dead code not detected before.In some cases, moved the definitions into an #if branch where it's actually used.
| @param version The optional version of QR code (by default - maximum possible depending on | ||
| @val version The optional version of QR code (by default - maximum possible depending on | ||
| the length of the string). | ||
| @param correction_level The optional level of error correction (by default - the lowest). | ||
| @param mode The optional encoding mode - Numeric, Alphanumeric, Byte, Kanji, ECI or Structured Append. | ||
| @param structure_number The optional number of QR codes to generate in Structured Append mode. | ||
| @val correction_level The optional level of error correction (by default - the lowest). | ||
| @val mode The optional encoding mode - Numeric, Alphanumeric, Byte, Kanji, ECI or Structured Append. | ||
| @val structure_number The optional number of QR codes to generate in Structured Append mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It breaks doxygen documentation.
asmorkalov left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
seanm commentedOct 6, 2023
Thanks guys! |
* Fixed clang -Wnewline-eof warnings* Fixed all trivial clang -Wextra-semi and -Wc++98-compat-extra-semi warnings* Removed trailing semi from various macros* Fixed various -Wunused-macros warnings* Fixed some trivial -Wdocumentation warnings* Fixed some -Wdocumentation-deprecated-sync warnings* Fixed incorrect indentation* Suppressed some clang warnings in 3rd party code* Fixed QRCodeEncoder::Params documentation.---------Co-authored-by: Alexander Smorkalov <alexander.smorkalov@xperience.ai>
* Fixed clang -Wnewline-eof warnings* Fixed all trivial clang -Wextra-semi and -Wc++98-compat-extra-semi warnings* Removed trailing semi from various macros* Fixed various -Wunused-macros warnings* Fixed some trivial -Wdocumentation warnings* Fixed some -Wdocumentation-deprecated-sync warnings* Fixed incorrect indentation* Suppressed some clang warnings in 3rd party code* Fixed QRCodeEncoder::Params documentation.---------Co-authored-by: Alexander Smorkalov <alexander.smorkalov@xperience.ai>
* Fixed clang -Wnewline-eof warnings* Fixed all trivial clang -Wextra-semi and -Wc++98-compat-extra-semi warnings* Removed trailing semi from various macros* Fixed various -Wunused-macros warnings* Fixed some trivial -Wdocumentation warnings* Fixed some -Wdocumentation-deprecated-sync warnings* Fixed incorrect indentation* Suppressed some clang warnings in 3rd party code* Fixed QRCodeEncoder::Params documentation.---------Co-authored-by: Alexander Smorkalov <alexander.smorkalov@xperience.ai>
No description provided.