Adar Dembo has posted comments on this change. Change subject: client.h: doxygen comments for C++ API ......................................................................
Patch Set 5: (9 comments) I didn't review the changes in client.h in detail yet, wanted to figure out the infrastructure first. Did you look into lint-style checkers that will warn (and, more importantly, fail a precommit build) if the doxygen syntax is malformed in the whitelisted files where it's expected? I think such a checker is important; without it, the syntax will gradually bitrot as people make mistakes, or code reviewers will have to look for syntactic mistakes. http://gerrit.cloudera.org:8080/#/c/3619/5/CMakeLists.txt File CMakeLists.txt: Line 963: find_package(Doxygen REQUIRED) Doesn't this mean that any invocation of cmake will require doxygen? That seems unnecessarily harsh to developers who don't want to build the docs. Line 966: WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/docs) Shouldn't this be CMAKE_CURRENT_BINARY_DIR? Otherwise we're polluting the source directory with "build output". Line 977: DEPENDS doxydocs) Does this work? The cmake docs say that DEPENDS should reference files and outputs of add_custom_command(), which suggests that the right way to enforce dependencies is via add_dependencies(). http://gerrit.cloudera.org:8080/#/c/3619/5/cmake_modules/FindDoxygen.cmake File cmake_modules/FindDoxygen.cmake: FindDoxygen.cmake is already in the cmake we distribute as part of thirdparty. Why duplicate it here? http://gerrit.cloudera.org:8080/#/c/3619/5/docs/.gitignore File docs/.gitignore: Line 19: /doxygen/ This file should no longer be necessary; we shouldn't be putting any doc output in the source directory. It all goes into the build directory. http://gerrit.cloudera.org:8080/#/c/3619/5/docs/support/doxygen/client.doxy File docs/support/doxygen/client.doxy: Line 21: # Doxyfile 1.8.10 What's the significance of the version? If there is some significance, perhaps explain in a comment? Otherwise, remove this comment? Line 26: These are all the parameters with non-default values, right? Could you add a comment to each justifying the non-default value choice? Line 29: INPUT = ../src/kudu/client/client.h The client API surface is more than just this one file. It made sense to start with client.h to prove the concept, but having done that, I think we should roll this out for the entire public-facing client API. The file set is defined in src/kudu/client/CMakeLists.txt, in the 'install' target. Ideally we wouldn't have to duplicate the list of files here; I was thinking the doxygen target could run 'make install' to some temporary directory, then invoke doxygen recursively on all of the headers within. http://gerrit.cloudera.org:8080/#/c/3619/5/src/kudu/client/client.h File src/kudu/client/client.h: Line 124: /// Signal number to use for internal Gerrit has flagged the trailing whitespace here, and below in the file. Could you trim? -- To view, visit http://gerrit.cloudera.org:8080/3619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-HasComments: Yes
