You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/05/13 00:05:13 UTC
[GitHub] [trafficserver] bneradt opened a new pull request #7829: Apply fmt compile time argument checking to log functions
bneradt opened a new pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829
This applies compile time argument checking for the log functions (diag,
debug, error, etc.).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7829: Apply fmt compile time argument checking to log functions
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829#discussion_r631487224
##########
File path: include/tscore/HTTPVersion.h
##########
@@ -40,7 +40,7 @@ class HTTPVersion
uint8_t get_major() const;
uint8_t get_minor() const;
- int get_flat_version() const;
+ uint32_t get_flat_version() const;
Review comment:
Is this really part of your PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7829: Apply fmt compile time argument checking to log functions
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829#discussion_r631536826
##########
File path: include/ts/apidefs.h.in
##########
@@ -51,11 +51,65 @@
#define tsapi
#endif
+/** Apply printf format string compile-time argument checking to a function.
+ *
+ *
+ * For example, given the following function from DiagsTypes.h:
+ *
+ * @code
+ * class Diags {
+ *
+ * ...
+ *
+ * void
+ * print(
+ * const char *tag,
+ * DiagsLevel level,
+ * const SourceLocation *loc,
+ * const char *fmt,
+ * ...) const;
+ *
+ * ...
+ *
+ * };
+ * @endcode
+ *
+ * This macro can be used to apply compiler checking for ... against the fmt
+ * argument like so:
+ *
+ *
+ * @code
+ * class Diags {
+ *
+ * ...
+ *
+ * void
+ * print(
+ * const char *tag,
+ * DiagsLevel level,
+ * const SourceLocation *loc,
+ * const char *fmt,
+ * ...) const TS_PRINTFLIKE(5, 6);
+ *
+ * ...
+ *
+ * };
+ * @endcode
+ *
+ * Note in this case, (5, 6) rather than (4, 5) is passed because `this`
+ * counts as the implicit first parameter of this member function.
+ *
+ * @fmt_index The index of the format string argument, with argument indexing
Review comment:
Oh, I usually add `@param[in]` and missed that.
Thanks for catching this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7829: Apply fmt compile time argument checking to log functions
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829#discussion_r631524157
##########
File path: include/tscore/HTTPVersion.h
##########
@@ -40,7 +40,7 @@ class HTTPVersion
uint8_t get_major() const;
uint8_t get_minor() const;
- int get_flat_version() const;
+ uint32_t get_flat_version() const;
Review comment:
Yes, it coincides with the fix for the warning generated (and fixed in this PR) here:
https://github.com/apache/trafficserver/pull/7829/files#diff-7f286e5169bc2d28c13f1ceaf3bb482b36cdb0743fbac4040fd4cd5a9c0897c1R4034
`%x` takes an unsigned integer, so I changed this to an unsigned return.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7829: Apply fmt compile time argument checking to log functions
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829#issuecomment-840180702
This will be needed for 9.1.x. It does not cherry-pick cleanly however. I will create a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7829: Apply fmt compile time argument checking to log functions
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829#discussion_r631522724
##########
File path: include/ts/apidefs.h.in
##########
@@ -51,11 +51,62 @@
#define tsapi
#endif
+/** Apply printf format string compile-time argument checking to a function.
+ *
+ *
+ * For example, given the following function from DiagsTypes.h:
+ *
+ * class Diags {
Review comment:
Nice. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #7829: Apply fmt compile time argument checking to log functions
Posted by GitBox <gi...@apache.org>.
bneradt merged pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7829: Apply fmt compile time argument checking to log functions
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829#discussion_r631487048
##########
File path: include/ts/apidefs.h.in
##########
@@ -51,11 +51,62 @@
#define tsapi
#endif
+/** Apply printf format string compile-time argument checking to a function.
+ *
+ *
+ * For example, given the following function from DiagsTypes.h:
+ *
+ * class Diags {
Review comment:
See [`@code`](https://www.doxygen.nl/manual/commands.html#cmdcode), [`@endcode`](https://www.doxygen.nl/manual/commands.html#cmdendcode). [Example](https://github.com/apache/trafficserver/blob/master/include/ts/ts.h#L104).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7829: Apply fmt compile time argument checking to log functions
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829#discussion_r631528935
##########
File path: include/ts/apidefs.h.in
##########
@@ -51,11 +51,65 @@
#define tsapi
#endif
+/** Apply printf format string compile-time argument checking to a function.
+ *
+ *
+ * For example, given the following function from DiagsTypes.h:
+ *
+ * @code
+ * class Diags {
+ *
+ * ...
+ *
+ * void
+ * print(
+ * const char *tag,
+ * DiagsLevel level,
+ * const SourceLocation *loc,
+ * const char *fmt,
+ * ...) const;
+ *
+ * ...
+ *
+ * };
+ * @endcode
+ *
+ * This macro can be used to apply compiler checking for ... against the fmt
+ * argument like so:
+ *
+ *
+ * @code
+ * class Diags {
+ *
+ * ...
+ *
+ * void
+ * print(
+ * const char *tag,
+ * DiagsLevel level,
+ * const SourceLocation *loc,
+ * const char *fmt,
+ * ...) const TS_PRINTFLIKE(5, 6);
+ *
+ * ...
+ *
+ * };
+ * @endcode
+ *
+ * Note in this case, (5, 6) rather than (4, 5) is passed because `this`
+ * counts as the implicit first parameter of this member function.
+ *
+ * @fmt_index The index of the format string argument, with argument indexing
Review comment:
Missed this previously - should be
```
@param fmt_index The index ....
@param arg_idx The index ....
```
[Example](https://github.com/apache/trafficserver/blob/master/include/ts/ts.h#L213).
Doxygen formatting is always "@cmd arg" where "cmd" is a Doxygen. For instance, to refer to the parameter "fmt_index" you would use `@a fmt_index`. So "@fmt_indx" isn't write.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7829: Apply fmt compile time argument checking to log functions
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7829:
URL: https://github.com/apache/trafficserver/pull/7829#issuecomment-840683026
> This will be needed for 9.1.x. It does not cherry-pick cleanly however. I will create a separate PR.
My mistake: we initially put this in 9.1.x and then reverted it from there because we determined it better for this to just go to master. Therefore no other PR is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org