doc: Add comments to FIELD_* constants in proxy.h #279

pull ViniciusCestarii wants to merge 1 commits into bitcoin-core:master from ViniciusCestarii:doc/field-flags-comments changing 1 files +16 −5
  1. ViniciusCestarii commented at 8:03 PM on May 26, 2026: contributor

    Add documentation comments to the five FIELD_* bit flag constants in proxy.h, which were previously undocumented

  2. DrahtBot commented at 8:03 PM on May 26, 2026: none

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ryanofsky

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #288 (Create support branch for CI scripts, documentation, and examples by ryanofsky)
    • #274 (Add nonunix platform support by ryanofsky)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. in include/mp/proxy.h:308 in d70648079d 
     308 | -static constexpr int FIELD_OUT = 2;
 309 | -static constexpr int FIELD_OPTIONAL = 4;
 310 | -static constexpr int FIELD_REQUESTED = 8;
 311 | -static constexpr int FIELD_BOXED = 16;
 312 | +static constexpr int FIELD_IN = 1;        //!< Field is read from the Cap'n Proto Params struct (client -> server).
 313 | +static constexpr int FIELD_OUT = 2;       //!< Field is read from the Cap'n Proto Results struct (server -> client).
    ryanofsky commented at 8:08 PM on June 2, 2026:

    In commit "doc: Add comments to FIELD_* constants in proxy.h" (d70648079db6fac354c842e52a037f77bc29f102)

    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

  4. in include/mp/proxy.h:307 in d70648079d 
     303 | @@ -304,11 +304,11 @@ struct ProxyServerMethodTraits : public ProxyMethodTraits<MethodParams>
 304 |  {
 305 |  };
 306 |  
 307 | -static constexpr int FIELD_IN = 1;
 308 | -static constexpr int FIELD_OUT = 2;
 309 | -static constexpr int FIELD_OPTIONAL = 4;
 310 | -static constexpr int FIELD_REQUESTED = 8;
 311 | -static constexpr int FIELD_BOXED = 16;
 312 | +static constexpr int FIELD_IN = 1;        //!< Field is read from the Cap'n Proto Params struct (client -> server).
    ryanofsky commented at 8:27 PM on June 2, 2026:

    In commit "doc: Add comments to FIELD_* constants in proxy.h" (d70648079db6fac354c842e52a037f77bc29f102)

    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.

  5. in include/mp/proxy.h:309 in d70648079d 
     309 | -static constexpr int FIELD_OPTIONAL = 4;
 310 | -static constexpr int FIELD_REQUESTED = 8;
 311 | -static constexpr int FIELD_BOXED = 16;
 312 | +static constexpr int FIELD_IN = 1;        //!< Field is read from the Cap'n Proto Params struct (client -> server).
 313 | +static constexpr int FIELD_OUT = 2;       //!< Field is read from the Cap'n Proto Results struct (server -> client).
 314 | +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()`.
    ryanofsky commented at 8:34 PM on June 2, 2026:

    In commit "doc: Add comments to FIELD_* constants in proxy.h" (d70648079db6fac354c842e52a037f77bc29f102)

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

  6. in include/mp/proxy.h:310 in d70648079d 
     310 | -static constexpr int FIELD_REQUESTED = 8;
 311 | -static constexpr int FIELD_BOXED = 16;
 312 | +static constexpr int FIELD_IN = 1;        //!< Field is read from the Cap'n Proto Params struct (client -> server).
 313 | +static constexpr int FIELD_OUT = 2;       //!< Field is read from the Cap'n Proto Results struct (server -> client).
 314 | +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()`.
 315 | +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.
    ryanofsky commented at 8:41 PM on June 2, 2026:

    In commit "doc: Add comments to FIELD_* constants in proxy.h" (d70648079db6fac354c842e52a037f77bc29f102)

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

  7. in include/mp/proxy.h:311 in d70648079d 
     311 | -static constexpr int FIELD_BOXED = 16;
 312 | +static constexpr int FIELD_IN = 1;        //!< Field is read from the Cap'n Proto Params struct (client -> server).
 313 | +static constexpr int FIELD_OUT = 2;       //!< Field is read from the Cap'n Proto Results struct (server -> client).
 314 | +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()`.
 315 | +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.
 316 | +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()`.
    ryanofsky commented at 8:45 PM on June 2, 2026:

    In commit "doc: Add comments to FIELD_* constants in proxy.h" (d70648079db6fac354c842e52a037f77bc29f102)

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

  8. ryanofsky commented at 8:53 PM on June 2, 2026: collaborator

    Code review d70648079db6fac354c842e52a037f77bc29f102. These comments are helpful but could be misleading in a few ways so I left some recommendations below.

  9. doc: Add comments to FIELD_* constants in proxy.h e863c6cdf6
  10. ViniciusCestarii force-pushed on Jun 3, 2026
  11. ViniciusCestarii commented at 1:32 PM on June 3, 2026: contributor

    Forced-push e863c6cdf61437485012fc4219bc9dd8e20b218f to add suggested changes

  12. in include/mp/proxy.h:317 in e863c6cdf6 
     317 |  
 318 |  //! Accessor type holding flags that determine how to access a message field.
 319 |  template <typename Field, int flags>
 320 |  struct Accessor : public Field
 321 |  {
 322 | +    //! Field is present from the Cap'n Proto Params struct (client -> server).
    ryanofsky commented at 7:54 PM on June 9, 2026:

    In commit "doc: Add comments to FIELD_* constants in proxy.h" (e863c6cdf61437485012fc4219bc9dd8e20b218f)

    IMO "present in" would sound more natural than "present from" but this seems ok

    ViniciusCestarii commented at 12:38 PM on June 10, 2026:

    I agree. Thanks for the feedback

  13. ryanofsky commented at 7:54 PM on June 9, 2026: collaborator

    Code review ACK e863c6cdf61437485012fc4219bc9dd8e20b218f. Thanks for the comments!

  14. ryanofsky merged this on Jun 9, 2026
  15. ryanofsky closed this on Jun 9, 2026
Contributors
ViniciusCestariiDrahtBotryanofsky
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/libmultiprocess. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-06-24 04:30 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me