Alexey Serbin has posted comments on this change.

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

Patch Set 5:


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 site pretty soon.  The style checkers could be added 
in background (but yes, it's important to have them in the long run).
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.

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

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.
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
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: Anonymous Coward #206
Gerrit-Reviewer: Jean-Daniel Cryans <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-HasComments: Yes

Reply via email to