Adar Dembo has posted comments on this change.

Change subject: client.h: doxygen comments for C++ API

Patch Set 5:


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.
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.

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().
File cmake_modules/FindDoxygen.cmake:

FindDoxygen.cmake is already in the cmake we distribute as part of thirdparty. 
Why duplicate it here?
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.
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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Jean-Daniel Cryans <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-HasComments: Yes

Reply via email to