doc: Add comments to FIELD_* constants in proxy.h#279
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
| static constexpr int FIELD_IN = 1; //!< Field is read from the Cap'n Proto Params struct (client -> server). | ||
| static constexpr int FIELD_OUT = 2; //!< Field is read from the Cap'n Proto Results struct (server -> client). |
There was a problem hiding this comment.
In commit "doc: Add comments to FIELD_* constants in proxy.h" (d706480)
I think it would be better to say "present" instead of "read". "read" is accurate but these flags are used to decide whether to write fields as well, so "present" reflects the wider context
| static constexpr int FIELD_OPTIONAL = 4; | ||
| static constexpr int FIELD_REQUESTED = 8; | ||
| static constexpr int FIELD_BOXED = 16; | ||
| static constexpr int FIELD_IN = 1; //!< Field is read from the Cap'n Proto Params struct (client -> server). |
There was a problem hiding this comment.
In commit "doc: Add comments to FIELD_* constants in proxy.h" (d706480)
I think it would be better to move these comments to the Accessor struct below where they are used, and written as preceding comments (//!) trailing comments (//!<) so there is more space for the descriptions.
Could keep //!< comments here referring to the Accessor struct below though.
| static constexpr int FIELD_BOXED = 16; | ||
| static constexpr int FIELD_IN = 1; //!< Field is read from the Cap'n Proto Params struct (client -> server). | ||
| static constexpr int FIELD_OUT = 2; //!< Field is read from the Cap'n Proto Results struct (server -> client). | ||
| static constexpr int FIELD_OPTIONAL = 4; //!< Field has a `has<Field>` sibling in the Cap'n Proto schema; value may be absent and presence must be checked via `has()`. |
There was a problem hiding this comment.
In commit "doc: Add comments to FIELD_* constants in proxy.h" (d706480)
Would replace the word "schema" with "struct" to be more specific. Also has<Field> looks like c++ template syntax which could be confusing. And I'm not sure the "presence must be checked" comment is meaningful because it seems focused on reading the field when the flag controls how the field needs to be written as well as read.
Would suggest something more like "Field has a companion has{Name} boolean field in the Cap'n Proto struct. This used to represent optional primitive values (e.g. C++ std::optional<int>) because Cap'n Proto doesn't allow primitive fields to be unset."
| static constexpr int FIELD_IN = 1; //!< Field is read from the Cap'n Proto Params struct (client -> server). | ||
| static constexpr int FIELD_OUT = 2; //!< Field is read from the Cap'n Proto Results struct (server -> client). | ||
| static constexpr int FIELD_OPTIONAL = 4; //!< Field has a `has<Field>` sibling in the Cap'n Proto schema; value may be absent and presence must be checked via `has()`. | ||
| static constexpr int FIELD_REQUESTED = 8; //!< Field has a `want<Field>` sibling in the Cap'n Proto Params struct; client opts in to receiving it. |
There was a problem hiding this comment.
In commit "doc: Add comments to FIELD_* constants in proxy.h" (d706480)
This doesn't really say what the flag is used for. Maybe would write "Results field has a companion want{Name} boolean field in the Params struct. Used for optional output parameters (e.g. C++ int*) and set to true if the caller passed a non-null and wants the result."
| static constexpr int FIELD_OUT = 2; //!< Field is read from the Cap'n Proto Results struct (server -> client). | ||
| static constexpr int FIELD_OPTIONAL = 4; //!< Field has a `has<Field>` sibling in the Cap'n Proto schema; value may be absent and presence must be checked via `has()`. | ||
| static constexpr int FIELD_REQUESTED = 8; //!< Field has a `want<Field>` sibling in the Cap'n Proto Params struct; client opts in to receiving it. | ||
| static constexpr int FIELD_BOXED = 16; //!< Field is a Cap'n Proto pointer type (struct, list, text, data, interface); value may be absent and presence must be checked via `has()`. |
There was a problem hiding this comment.
In commit "doc: Add comments to FIELD_* constants in proxy.h" (d706480)
I think absense/presence is not really relevant here. Would suggest something more like "Field is a Cap'n Proto pointer type (struct, list, text, data, interface) as opposed to a primitive type (bool, int, float, enum)."
ViniciusCestarii
force-pushed
the
doc/field-flags-comments
branch
from
d706480 to
e863c6c
Compare
|
Forced-push e863c6c to add suggested changes |
3 participants
Add documentation comments to the five
FIELD_*bit flag constants inproxy.h, which were previously undocumented