Alexey Serbin has posted comments on this change.

Change subject: [C++ client] doxygenized all C++ client API
......................................................................


Patch Set 2:

(85 comments)

Thank you for the review!  Will post the updated patch soon.

http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/callbacks.h
File src/kudu/client/callbacks.h:

Line 60:   ///   The absolute time when the loggin event is generated.
> "when the log event was"
Done


Line 62:   ///   The message to log.  It's not terminated with an endline.
> Nit: you have two spaces here after the period.
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 309:   ///   Result parameter @c tables is appended only on success.
> Maybe "The placeholder for the result. Appended only on success"?
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/resource_metrics.h
File src/kudu/client/resource_metrics.h:

Line 45:   ///   The increment/decrement to aggregate.
> Maybe "The amount to increment/decrement", with some mention of how amount 
Done


Line 48:   /// Get current count for the specified metrics.
> "specified metric" since it's just one metric.
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:

Line 44: /// As a result of a scan operation, KuduScanner object returns batch 
of
> Maybe something like "Every call to KuduScanner::NextBatch() returns a batc
Done


Line 105:   ///   The returned object is only valid for as long as this 
KuduScanBatch.
> Nit: "for as long as this KuduScanBatch is valid"
Done


Line 127: class KUDU_EXPORT KuduScanBatch::RowPtr {
> A @brief for this?
RowPtr is documented by the forward declaration for RowPtr earlier.  For some 
reason, if documentation is placed here, it's not shown in a proper place, so I 
moved it to the place of the forward declaration of RowPtr in KuduScanBatch.


Line 136:   bool IsNull(const Slice& col_name) const;
> No summarizing sentences for the two IsNull() methods?
I thought it's not necessary here since we have self-descriptive @return doc.  
That's one of those cases where @brief (or summary) would just re-iterate 
whatever @return states (and vice versa).


Line 155:   ///@{
> Clever.
I was scared of duplicating that stuff for every method, so I decided to try 
this and it worked.  Nice to have this functionality in doxygen.


Line 172:   /// since using indices avoid a hashmap lookup, so index-based 
getters
> Nit: avoids. Below too.
Done


PS2, Line 179:   ///   Note that the method does not copy the value. Callers 
should copy
             :   ///   the resulting Slice if necessary.
> Doesn't seem right for these getters.
Done


Line 201:   /// @name Getters for string/binary column by column name
> Nit: missing terminating period. Below too.
Done


PS2, Line 255: -- useful
             :   ///   for outputting information into the log.
> Nit: just drop the addendum; "string representation..." is sufficient.
Done


Line 281: class KUDU_EXPORT KuduScanBatch::const_iterator
> No @brief?
Here we have the same situation as with KuduScanBatch::RowPtr -- the 
documentation for this is tied with the forward-declared 
KuduScanBatch::const_iterator.


Line 311:   ///   of the same batch row.
> Nit: "of the same batch"
Done


Line 312:   bool operator==(const const_iterator& other) const {
> No summarizing sentence for this or operator!= ?
Done


PS2, Line 318: different row
             :   ///   of or associated with a different batch row.
> Maybe "different row in the same batch, or to a row belonging to a differen
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/schema.h
File src/kudu/client/schema.h:

Line 151:   ///   Column attributes.
> Nit: Column storage attributes.
Done


Line 158:   /// Construct KuduColumnSchema object as a copy of other object.
> Nit: "as a copy from another object".
For some reason, doxygen tends to show documentation for copy constructors and 
assignment operators, but not for default constructors and destructor.  If 
there were a configuration directive to change this, it would be nice, but I 
could not find such a configuration directive.


Line 172:   /// Make this object an identical copy of other one.
> Nit: "of the other one."
Done


Line 178:   /// Check whether the object is identical to the other.
> Nit: "identical to the other one."
Done


PS2, Line 229: When inserting a new row with no value for this column,
             :   /// the default value will also be used.
> Nit: "The default value will also be used when inserting a new row with no 
Done


Line 233:   ///   The value to use as the default.  The KuduColumnSpec takes 
ownership
> Got two spaces after the period here.
Done


Line 239:   /// Set the preferred compression for the column.
> Nit: "compression type"
Done


Line 242:   ///   The compression to use.
> Nit: "compression type"
Done


Line 279:   /// Set this column to be the primary key of the table.
> Nit: "Set the column" to be consistent.
Done


Line 304:   /// Set the type of the column.
> Nit: "data type". Below too.
Done


Line 317:   /// Remove the default value for this column.
> Nit: "for the column"
Done


Line 380:   KuduColumnSpec* AddColumn(const std::string& name);
> Nit: summarizing sentence?
Done


Line 394:   ///   The placeholder for the result schema.  Upon successful 
completion,
> Nit: two spaces.
Done


Line 408: /// @brief A representation of schema.
> Nit: perhaps "of a table's schema"?
Done


Line 413:   /// Create a KuduSchema object as a copy of the other one.
> As I wrote in an earlier comment, do we have to document constructors/opera
I didn't find how to make doxygen to ignore adding copy constructors/assignment 
operators into the generated documentation.  So, copy the constructor and the 
assignment operator are added into the auto-generated docs and it's nice to 
have them documented.


Line 446:   bool Equals(const KuduSchema& other) const;
> Missing summarizing sentence for this and for Column() (which is arguably a
Done


Line 469:   ///   destroyed before the KuduSchema object to omit dangling 
pointers.
> Nit: "to avoid dangling pointers".
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/stubs.h
File src/kudu/client/stubs.h:

Line 160: /// @brief A helper for nil log sink.
> Should explain that the "nil log sink" means nothing gets logged.
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:

Line 66:   /// @note To work with a row, use KuduPartialRow API for field 
getters, etc.
> Nit: "use the KuduPartialRow..." Below too.
Done


PS2, Line 74:   Useful for outputting
            :   ///   information into log, etc.
> Nit: no need for this.
Done


Line 84:   explicit KuduWriteOperation(const sp::shared_ptr<KuduTable>& table);
> The protected members aren't accessible to users, so why bother documenting
This is a just trick to avoid warnings from doxygen on undocumented members.  
I'd like to have not a single warning output by doxygen (now we can turn on the 
detection of those warnings in the makefile and catch un-documented members, 
etc.).

The doxygen treats protected members as a part of interface to be documented.  
The reasoning is that if someone is about to inherit from the class, it's nice 
to have the accessible members of the base class documented.

Notice the @cond ... statement: effectively, we have no documentation generated 
for these protected members since the PROTECTED_MEMBERS_DOCUMENTED is set to 
NO/false.


Line 173: ///   in the schema to be set in the embedded PartialRow object.
> Nit: KuduPartialRow. Same with KuduDelete.
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:

PS2, Line 50: This type
> This is a strange way to refer to the object. Maybe "This object"? Or "A Ku
Done


Line 62:   /// Create a copy of KuduPartialRow instance.
> Do we really need to document these?
Yes -- as I mentioned earlier, the copy constructor and the assignment operator 
should be documented.  Otherwise, there will be a warning from doxygen.  I 
don't know how to change this behavior in current version of doxygen -- may be, 
in the future versions there will be some configuration switch to change this.


PS2, Line 75:   //------------------------------------------------------------
            :   // Setters
            :   //------------------------------------------------------------
> How does doxygen treat these separators?
It ignores these.  I'll remove them.


PS2, Line 106: The same a
> Nit: "These setters are the same as the corresponding ...".
Done


PS2, Line 151: The same as
> Nit: "These setters are the same as the corresponding ...".
Done


Line 250:   bool IsColumnSet(const Slice& col_name) const;
> Do the IsColumnSet() and IsNull() methods deserve summarizing sentences?
Done


PS2, Line 259: the given column has @c NULL value.
> Nit: "the given column's value is @c NULL". Below too.
Done


PS2, Line 295: The same as
> Nit: "These getters are the same as the corresponding ...".
Done


Line 346:   /// since using indices avoid a hashmap lookup, so index-based 
getters
> "since they use indices to avoid hashmap lookups".
Done


Line 379:   /// @return Operation result status.  In particular, returns 
InvalidArgument
> Nit: two spaces here.
Done


PS2, Line 395: if all of the key columns have been specified
             :   ///   for this mutation.
> Nit" If all key column values have been set."
Done


Line 399:   /// @return @c true if all columns have been specified.
> Nit: "if all column values have been set."
Done


PS2, Line 402: useful
             :   ///   for outputting information into the log.
> Nit: drop.
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/util/monotime.h
File src/kudu/util/monotime.h:

Line 39: /// @brief A representation of time interval.
> Nit: "of a time interval"
Done


PS2, Line 110:   /// @note If MonoDelta::IsPositive() returns true, the struct 
timeval is
             :   ///   guaranteed to hold a positive number as well (at least 1 
microsecond).
> Is this still relevant? I don't see MonoDelta::IsPositive(); we must have r
Done


Line 117:   /// Represent this time interval as a timespec structure.
> With nanonsecond accuracy.
Done


Line 152:   /// accurate to within a millisecond or two.  The speed difference 
will
> Nit: two spaces.
Done


Line 159:   /// @name Convertion constants for ubiquitous time units.
> "Conversion"
Done


Line 187:   static const MonoTime& Earliest(const MonoTime& a, const MonoTime& 
b);
> Nit: summarizing sentence?
Done


Line 200:   ///   The object to specify the left boundary of the time interval,
> "The object that corresponds to the left boundary of the time interval"
Done


Line 228:   ///   is the same as represented by the other.
> Nit: "is the same as the one..."
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/util/slice.h
File src/kudu/util/slice.h:

PS2, Line 40: /// Slices can be built around faststrings and StringPieces using 
constructors
            : /// with implicit casts. Both StringPieces and faststrings depend 
on a great
            : /// deal of gutil code, so these constructors are conditionalized 
on
            : /// KUDU_HEADERS_USE_RICH_SLICE. Likewise, 
KUDU_HEADERS_USE_RICH_SLICE controls
            : /// whether to use gutil-based memeq/memcmp substitutes; if it is 
unset, Slice
            : /// will fall back to standard memcmp.
> Clients never define KUDU_HEADERS_USE_RICH_SLICE, so I wonder whether we sh
Done


Line 52:   /// Create a slice that refers to a @c uint8_t array.
> Nit: maybe "to a @c uint8_t byte array"? Below too. Then "Number of element
Done


Line 100:   /// @note Further appends to the faststring may invalidate this 
slice.
> Does this apply to StringPieces too?
It seems it does not.  I'll remove this.


Line 126:   /// @return the ith byte in the referenced data.
> Nit: nth byte?
Done


Line 132:   /// Change this slice to refer to an empty array
> Nit: terminating period.
Done


Line 143:   ///   Number of bytes to drop/skip in the beginning.
> Nit: "Number of bytes that should be dropped".
Done


Line 155:   ///   The number of bytes to have after the truncation.
> Nit: "New size of the slice."
Done


Line 161:   /// Check the slice has the expected size.
> Nit: "Check that the slice..."
Done


PS2, Line 164: Status::OK iff size() == @c expected_size;
             :   ///   Status::Corruption() otherwise.
> How about just "Corruption iff size() != @c expected_size"?
Done


Line 168:   /// @return A string that contains the copy of the referenced data.
> Nit: "...contains a copy of the..."
Done


Line 175:   ///   @c 0 means there is no limit.
> Nit: "@c 0 means no limit"
Done


Line 179:   /// Do three-way comparison of the slice's data.
> Nit: "Do a three-way comparison..."
Done


Line 256: /// @return @c true iff two slices contain byte-to-byte identical 
data.
> Nit: "byte-for-byte"
Done


PS2, Line 275: /// This method is useful in debug and troubleshooting scenarios 
when it's
             : /// necessary to output slice information in printable form into 
log file, etc.
> Eh, I think this is self-explanatory.
Done


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/util/status.h
File src/kudu/util/status.h:

Line 103: /// @brief The class to represent outcome of performed operation.
> Nit: "Representation of an operation's outcome"
Done


Line 111:   /// Copy the specified status.
> Do we need these?
Yep -- to avoid doxygen warnings in with our current configuration.


Line 130:   /// Assign the specified status using the move semantics (C++11).
> Nit: "using move semantics"
Done


Line 241:   /// @return @c true iff the status indicates an InvalidArgument 
error
> Nit: terminate with period. Below too.
Done


Line 253:   /// @return @c true iff the status indicates a IllegalState.
> Nit: an IllegalState.
Done


Line 256:   /// @return @c true iff the status indicates a NotAuthorized.
> Nit: NotAuthorized error.
Done


Line 262:   /// @return @c true iff the status indicates RemoteError.
> Nit: a RemoteError.
Done


Line 274:   /// @return @c true iff the status indicates Configuration error.
> Nit: ConfigurationError.
Done


Line 297:   /// @return The message portion of the Status.  For @c OK statuses,
> Nit: two spaces
Done


Line 309:   Status CloneAndPrepend(const Slice& msg) const;
> Summarizing sentence? Below too.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/3840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
Gerrit-PatchSet: 2
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: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to