Alexey Serbin has posted comments on this change. Change subject: client.h: doxygen comments for C++ API ......................................................................
Patch Set 5: (9 comments) Thank you for the review! I took a look at vera++ as a candidate for syntax rule/style enforcement. It will require to write a couple of scripts in TCL for that, but I haven't done that part yet. However, I used vera++ for other projects and it worked fine for me. I haven't looked for other stylecheckers besides vera++. As I understood, syntax/style checker is only the thing we want to have, since we don't want to enforce documentation of every parameter and return type or documenting every public member of public C++ interface. Here the idea was to roll out current file, pushing auto-generated docs for C++ API to the kudu.apache.org site pretty soon. The style checkers could be added in background (but yes, it's important to have them in the long run). 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 s Yes, you are right -- cmake will require doxygen to be present. And yes, it might be too harsh if requiring it as installed by default. However, my idea is to build the doxygen as a third-party tool (like, say curl) and I have that change locally but I haven't posted it yet. I'm awaiting access to be granted to the Cloudfront repo to upload doxygen source tarball. Sorry for not posting it yet. Probably, it's also possible to search for doxygen and build it only upon building the 'doxydocs' target. I'll take a look on that. Line 966: WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/docs) > Shouldn't this be CMAKE_CURRENT_BINARY_DIR? Otherwise we're polluting the s Yep, that's a typo. Thanks -- will fix. Line 977: DEPENDS doxydocs) > Does this work? The cmake docs say that DEPENDS should reference files and Yes, it works. In the result GNU makefile the docs rule runs doxydoc-related stuff by dependency. But I'll re-do it anyways. Thanks for pointing at that. 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 thirdpar Frankly, I didn't think about this -- I just followed the same scheme as for GTest, ZLib and others. Ok, I will take a look to clarify on this. 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, perh Originally, the file was generated as a template by doxygen, and it inserted this version here. I was thinking about using this as a sort of hint for up to which version of doxygen this file is valid and which parameter to be expected as default/non-default, but it appears to be a bad idea after some consideration. Will remove this. Line 26: > These are all the parameters with non-default values, right? Could you add Right, for that particular version of doxygen they are non-default. Yes, I'll add those comments -- it makes sense. Line 29: INPUT = ../src/kudu/client/client.h > The client API surface is more than just this one file. It made sense to st Yep, that makes sense -- it's good idea. Using mktemp, create a directory, install those files there, copy the doxyfile into that dir and then run doxygen from that dir. Excellent, will use that approach. 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. Cou Yep, will do. I forgot to update my .vimrc accordingly before editing this file -- it should have shown cspace errors. PS5, Line 197: is > "returned." is omitted here. Thanks! Will update. -- 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: Anonymous Coward #206 Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-HasComments: Yes
