Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1519113003:
fix doxygen for IceBuildDefs.h (Closed)
|
Created:
5 years ago by rkotlerimgtec Modified:
5 years ago CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Descriptionfix doxygen for IceBuildDefs.h
This is an attempt to make IceBuildDefs more understandable
in the Doxygen produced output, as well as when the various
functions are later referenced from other parts of the subzero docs.
If you look at the doxygen for both the file and the IceBuildDefs namespace, I think it's definitely much
clearer.
I'm still learning Doxygen and always see new things and more that I could have done but Rome was not built in a day.
In my browser:
file:///home/rkotler/nacl_dir/native_client/toolchain_build/src/subzero/docs/html/namespaceIce_1_1BuildDefs.html
and
file:///home/rkotler/nacl_dir/native_client/toolchain_build/src/subzero/docs/html/IceBuildDefs_8h.html
BUG=
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=2a18dd3fbd3c2012da8a08bd2c4d0f904a36360a
Patch Set 1 #Patch Set 2 : Fix 80 character line limits #
Total comments: 24
Patch Set 3 : changes suggested by stichnot #
Total comments: 8
Patch Set 4 : changes suggested by stichnot #Messages
Total messages: 12 (3 generated)
Description was changed from ========== fix doxygen for IceBuildDefs.h BUG= ========== to ========== fix doxygen for IceBuildDefs.h This is an attempt to make IceBuildDefs more understandable in the Doxygen produced output, as well as when the various functions are later referenced from other parts of the subzero docs. If you look at the doxygen for both the file and the IceBuildDefs namespace, I think it's definitely much clearer. I'm still learning Doxygen and always see new things and more that I could have done but Rome was not built in a day. In my browser: file:///home/rkotler/nacl_dir/native_client/toolchain_build/src/subzero/docs/html/namespaceIce_1_1BuildDefs.html and file:///home/rkotler/nacl_dir/native_client/toolchain_build/src/subzero/docs/html/IceBuildDefs_8h.html BUG= ==========
rkotlerimgtec@gmail.com changed reviewers: + jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org
On 2015/12/11 21:33:37, rkotlerimgtec wrote: I realized I forgot to check for 80 character line limits. Will resubmit.
https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h File src/IceBuildDefs.h (right): https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:11: /// \brief Define the Ice::BuildDefs Namespace lowercase "namespace" https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:18: /// \brief Defines constexpr functions that express various subzero build Capitalize Subzero https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:22: /// conditionally compiled without having to do this using the older c++ C++ https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:24: /** \verbatim It seems like this \verbatim section could/should start one paragraph earlier, after the \brief, but I don't know doxygen well enough. What do you think? https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:36: constexpr bool hasFeature() { return FEATURE_SUPPORTED; } I suggest adding a comment above, something like: // Use this form when FEATURE_SUPPORTED is guaranteed to be defined on the C++ compiler command line as 0 or 1. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:40: constexpr bool hasFeature() { Likewise, a comment like: // Use this form when FEATURE_SUPPORTED may not necessarily be defined on the C++ compiler command line. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:41: #ifdef FEATURE_SUPPORTED With one exception, Subzero uses "#if" instead of "#ifdef", so that if a specific -D is omitted, its value is taken as 0. (The one exception is NDEBUG which we don't really control.) I suggest using "#if" here for consistency. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:48: ...} // of BuildDefs Might as well use the same comment style as in the code base: // end of namespace BuildDefs https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:49: } // end of Ice Same here -- // end of namespace Ice https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:51: Depending on whether the build system gives a value always to Remove this paragraph if you agree with the corresponding 2 comments above. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:72: /// return true if ALLOW_DISABLE_IR_GEN is defined as a non-zero value Should "Return" be capitalized, here and below? https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:82: /// return true if ALLOW_MINIMUM is defined as a non-zero value ALLOW_MINIMAL_BUILD
https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h File src/IceBuildDefs.h (right): https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:11: /// \brief Define the Ice::BuildDefs Namespace On 2015/12/12 00:04:34, stichnot wrote: > lowercase "namespace" Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:18: /// \brief Defines constexpr functions that express various subzero build On 2015/12/12 00:04:33, stichnot wrote: > Capitalize Subzero Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:22: /// conditionally compiled without having to do this using the older c++ On 2015/12/12 00:04:34, stichnot wrote: > C++ Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:24: /** \verbatim On 2015/12/12 00:04:34, stichnot wrote: > It seems like this \verbatim section could/should start one paragraph earlier, > after the \brief, but I don't know doxygen well enough. What do you think? Verbatim does not flow at all. It's verbatim so I'm just using it here for code that I want to be formatted exactly as I typed it in. The verbatim should actually end earlier and revert to flow which I'm going to change it to do. If you make your browser window more narrow or wide you will see all the text flowing. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:36: constexpr bool hasFeature() { return FEATURE_SUPPORTED; } On 2015/12/12 00:04:33, stichnot wrote: > I suggest adding a comment above, something like: > > // Use this form when FEATURE_SUPPORTED is guaranteed to be defined on the C++ > compiler command line as 0 or 1. Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:40: constexpr bool hasFeature() { On 2015/12/12 00:04:34, stichnot wrote: > Likewise, a comment like: > > // Use this form when FEATURE_SUPPORTED may not necessarily be defined on the > C++ compiler command line. Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:41: #ifdef FEATURE_SUPPORTED On 2015/12/12 00:04:34, stichnot wrote: > With one exception, Subzero uses "#if" instead of "#ifdef", so that if a > specific -D is omitted, its value is taken as 0. (The one exception is NDEBUG > which we don't really control.) > > I suggest using "#if" here for consistency. Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:48: ...} // of BuildDefs On 2015/12/12 00:04:34, stichnot wrote: > Might as well use the same comment style as in the code base: > // end of namespace BuildDefs Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:49: } // end of Ice On 2015/12/12 00:04:33, stichnot wrote: > Same here -- > // end of namespace Ice Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:51: Depending on whether the build system gives a value always to On 2015/12/12 00:04:33, stichnot wrote: > Remove this paragraph if you agree with the corresponding 2 comments above. Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:72: /// return true if ALLOW_DISABLE_IR_GEN is defined as a non-zero value On 2015/12/12 00:04:33, stichnot wrote: > Should "Return" be capitalized, here and below? Done. https://codereview.chromium.org/1519113003/diff/20001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:82: /// return true if ALLOW_MINIMUM is defined as a non-zero value On 2015/12/12 00:04:34, stichnot wrote: > ALLOW_MINIMAL_BUILD Done.
https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h File src/IceBuildDefs.h (right): https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:34: We can have: How about a blank line between prose and sample code https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:36: namespace BuildDefs add "{" at the end of line https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:64: \endverbatim Blank line after this? https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:87: /// Return true if ALLOW_MINIMUM_BUILD is defined as a non-zero value ALLOW_MINIMAL_BUILD
https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h File src/IceBuildDefs.h (right): https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:34: We can have: On 2015/12/12 03:39:36, stichnot wrote: > How about a blank line between prose and sample code Done. https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:36: namespace BuildDefs On 2015/12/12 03:39:35, stichnot wrote: > add "{" at the end of line Done. https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:64: \endverbatim On 2015/12/12 03:39:35, stichnot wrote: > Blank line after this? Done. https://codereview.chromium.org/1519113003/diff/40001/src/IceBuildDefs.h#newc... src/IceBuildDefs.h:87: /// Return true if ALLOW_MINIMUM_BUILD is defined as a non-zero value On 2015/12/12 03:39:35, stichnot wrote: > ALLOW_MINIMAL_BUILD Done.
lgtm
Description was changed from ========== fix doxygen for IceBuildDefs.h This is an attempt to make IceBuildDefs more understandable in the Doxygen produced output, as well as when the various functions are later referenced from other parts of the subzero docs. If you look at the doxygen for both the file and the IceBuildDefs namespace, I think it's definitely much clearer. I'm still learning Doxygen and always see new things and more that I could have done but Rome was not built in a day. In my browser: file:///home/rkotler/nacl_dir/native_client/toolchain_build/src/subzero/docs/html/namespaceIce_1_1BuildDefs.html and file:///home/rkotler/nacl_dir/native_client/toolchain_build/src/subzero/docs/html/IceBuildDefs_8h.html BUG= ========== to ========== fix doxygen for IceBuildDefs.h This is an attempt to make IceBuildDefs more understandable in the Doxygen produced output, as well as when the various functions are later referenced from other parts of the subzero docs. If you look at the doxygen for both the file and the IceBuildDefs namespace, I think it's definitely much clearer. I'm still learning Doxygen and always see new things and more that I could have done but Rome was not built in a day. In my browser: file:///home/rkotler/nacl_dir/native_client/toolchain_build/src/subzero/docs/html/namespaceIce_1_1BuildDefs.html and file:///home/rkotler/nacl_dir/native_client/toolchain_build/src/subzero/docs/html/IceBuildDefs_8h.html BUG= R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 2a18dd3fbd3c2012da8a08bd2c4d0f904a36360a (presubmit successful). |
