Skip to content

CXX-3069 document headers and root namespace redeclarations (bsoncxx) #1182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Aug 5, 2024

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Jul 31, 2024

Summary

Partially resolves CXX-3069. Followup to #1173. Verified by this patch.

A followup PR will resolve CXX-3069 by applying the same pattern of changes in this PR to mongocxx.

A thorough audit of the documentation of interfaces will be addressed by CXX-3082.

File Documentation

This PR follows the pattern used in #1173 and adds @file Doxygen commands at the bottom of all public header files under include/. This is to avoid Doxygen-specific code from intruding in the code they are meant to document (+ they are not expected to require frequent maintenance).

// License notice.

#pragma once

#include <headers> // Include directives and etc.

namespace bsoncxx { ... } // Declarations and definitions.

///
/// @file
/// Documentation for this file.
///

///
/// More Doxygen-specific comments pertaining to entities provided by this file.
///

The result of these efforts is the complete documentation of all files in the File List page (for bsoncxx; mongocxx soon to follow).

🎉 This completes the trifecta of Doxygen API doc page navigation via namespaces, classes, and files. 🎉

Files list, before:

image

Files list, after:

image

File page, before: none.

File page, after:

image

Note

File pages serve as a collection of references to namespaces (and their immediate members) and classes provided by the header. Accordingly, documentation for interfaces should always be primarily associated with the namespace or class, not the file it is declared/documented in. In short, @file documentation should be very brief.

Root Namespace Redeclarations

As doxygen/doxygen#3760 is yet to be resolved, Doxygen is completely blind to using-declarations. Therefore, great pains are taken to reproduce the documentation structure generated by using namespace prior to #1066 and #1070, as demonstrated by the 3.9.0 docs:

image

This PR uses the #if defined(BSONCXX_PRIVATE_DOXYGEN_PREPROCESSOR) pattern implemented in #1173 to actually "redeclare" equivalents to the root namespace using-declarations as class/function stubs, solely to generate doc pages that redirect their aliased counterparts:

namespace bsoncxx {

using v_noabi::foo;

} // namespace bsoncxx

///
/// @file
/// Documentation for this file.
///

#if defined(BSONCXX_PRIVATE_DOXYGEN_PREPROCESSOR)

namespace bsoncxx {

/// @ref bsoncxx::v_noabi::foo
class foo {};

} // namespace bsoncxx

#endif // defined(BSONCXX_PRIVATE_DOXYGEN_PREPROCESSOR)

The result looks as follows (specifically the bsoncxx::array members at the top of the image):

image

This pattern has the unexpected benefit of explicitly displaying the redeclared entity as a clickable link in the brief description on the Namespace List page (instead of repeating the brief of the redeclared entity). This should hopefully make the state of redeclarations easier to follow than it would have otherwise been.

The Doxygen documentation for redeclarations always qualify entities with the ABI namespace. This is to avoid Doxygen associating types with a root namespace redeclaration (Doxygen does not appear to be smart enough to resolve namespace scoping rules for parameter types):

#if defined(BSONCXX_PRIVATE_DOXYGEN_PREPROCESSOR)

namespace bsoncxx {

/// @ref bsoncxx::v_noabi::bar
class bar {};

/// @ref bsoncxx::v_noabi::foo(bar b)
void foo(bar b);
//       ^~~ Should be `bsoncxx::v_noabi::bar`, not `bsoncxx::bar`!

} // namespace bsoncxx

#endif // defined(BSONCXX_PRIVATE_DOXYGEN_PREPROCESSOR)

This principle is similar to the one which motivated ABI namespace qualification of bsoncxx:: entities in mongocxx to ensure ABI stability across redeclaration updates, as described in #1070 (comment). A redeclaration changing from v1 -> v2 must not inadvertently change the meaning of interfaces in ABI-specific namespaces in either code or in documentation.

Deprecation, Internal Use, and Deliberate No-Documentation

This PR takes this opportunity to document bsoncxx/util/functor.hpp as deprecated (consistent with the directory-level deprecation notice).

Several header files are documented as "for internal use only" in an effort to discourage users from directly depending on certain interfaces:

  • bsoncxx/builder/basic/impl.hpp
  • bsoncxx/config/compiler.hpp
  • bsoncxx/config/util.hpp
  • bsoncxx/stdx/make_unique.hpp
  • bsoncxx/stdx/operators.hpp
  • bsoncxx/stdx/type_traits.hpp

These headers have been updated to deliberately contain no documentation of the provided entities in an effort to discourage their use by users. Doxygen-aware tools appear to have no issue parsing and treating // (double, not triple) comments as Doxygen documentation despite not being included in doc generation; therefore, existing documentation of internal interfaces have been "demoted" from /// or /** to // to avoid their inclusion in generated docs.

For example, this is the entirety of the type_traits.hpp doc page:

image

On the other hand, given the significance of std::string_view and std::optional<T> polyfills in C++ Driver interfaces, these headers (and the polyfills) are given special documentation with a note regarding their relationship with polyfill library configuration. This documentation is expected to be expanded in a followup PR.

Some old/niche/undocumented features have been given the Deliberately undocumented. note to avoid their inclusion in generated docs.

Doxygen Parsing Problems

The operators.hpp and type_traits.hpp headers were known to give Doxygen trouble, hence their explicit exclusion via the EXCLUDE Doxygen option in #1062. This PR reverts their exclusion and instead makes adjustments for consistency with other (internal use) header files and applying the @cond DOXYGEN_DISABLE pattern (first used in #1170) to address Doxygen warnings. These changes address three Found ';' while parsing initializer list! warnings and four Detected potential recursive class relation warnings.

bsoncxx::v_noabi::error_code

As a drive-by improvement, identified and (partially) fixed misplaced documentation for enumerators of bsoncxx::v_noabi::error_code.

Before:

image

After:

image

Addressing the missing documentation for enumerators relating to BSON types and binary subtypes is deferred to a followup PR.

Miscellaneous

Warnings-related fixes are a followup to #1178 that extends compilation output to the test_bson and test_driver targets.

  • Drive-by formatting fix to mongocxx/test/collection.cpp following d373213.
  • Addressed 617 (8 unique) -Wzero-as-null-pointer-constant warnings by operators.hpp and catch.hh.
  • Addressed 92 (69 unique) -Wundefined-func-template warnings emitted by use of soft_declval (replaced with equivalent std::declval expressions).
  • Silenced 33 (8 unique) -Wfloat-equal warnings emitted by optional.hpp.
  • Address a -Wundef warning in bsoncxx/test/string_view.test.cpp.
  • Silenced a -Wweak-vtables warning in optional.hpp (we do not export symbols in stdx).
  • Set EXAMPLES = examples in Doxyfile (setup for CXX-3082).

@eramongodb eramongodb requested a review from kevinAlbs July 31, 2024 21:39
@eramongodb eramongodb self-assigned this Jul 31, 2024

/// @ref bsoncxx::v_noabi::builder::basic::make_array
template <typename... Args>
v_noabi::array::value make_array(Args&&... args);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming: Is mixing ABI namespace and root namespace discouraged?

// Do this:
bsoncxx::foo f = bsoncxx::get_foo();
// Or even better:
auto f = bsoncxx::get_foo();
// Not this:
bsoncxx::v_noabi::foo f = bsoncxx::get_foo();

I expect using the root namespace helps avoid source code changes on ABI incompatible (but API compatible) driver changes. However, mixing may result in more needed source code changes (e.g. if bsoncxx::get_foo() later defined to return bsoncxx::v1::foo).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is mixing ABI namespace and root namespace discouraged?

Yes, for the reason you described regarding bsoncxx::get_foo() returning bsoncxx::v1::foo.

However, old ABI namespace interfaces are expected to provide conversion functions to/from the new ABI namespace interfaces, which will assist with incremental migration across ABI namespaces. This is already being implemented for CXX-2745.

// OK: implicit conversion from `v1::foo` to `v_noabi::foo`
// as defined by `v_noabi::foo` (implicit constructor).
v_noabi::foo f = v1::get_foo();

// OK: explicit conversion from `v1::foo` to `v_noabi::foo`
// via the `v_noabi` conversion function.
v_noabi::foo f = v_noabi::from_v1(v1::get_foo());

// error: from v1 to what? (ADL not supported.)
v_noabi::foo f = from_v1(v1::get_foo());

// error: conversion from `v_noabi::foo` to `v1::foo`
// must be explicit (one-way implicit conversions).
v1::foo f = v_noabi::get_foo();

// OK: explicit forward-compatibility via conversion function.
v1::foo f = v_noabi::to_v1(v_noabi::get_foo());

// OK: same as above, but found via ADL.
v1::foo f = to_v1(v_noabi::get_foo());

The (implicit) conversions will of course come with a performance cost, so relying on them too much is not recommended. However, this will faciliate incremental migration from vA to vB both in user code and in internal code.

Copy link
Contributor Author

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes verified by this patch.


/// @ref bsoncxx::v_noabi::builder::basic::make_array
template <typename... Args>
v_noabi::array::value make_array(Args&&... args);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is mixing ABI namespace and root namespace discouraged?

Yes, for the reason you described regarding bsoncxx::get_foo() returning bsoncxx::v1::foo.

However, old ABI namespace interfaces are expected to provide conversion functions to/from the new ABI namespace interfaces, which will assist with incremental migration across ABI namespaces. This is already being implemented for CXX-2745.

// OK: implicit conversion from `v1::foo` to `v_noabi::foo`
// as defined by `v_noabi::foo` (implicit constructor).
v_noabi::foo f = v1::get_foo();

// OK: explicit conversion from `v1::foo` to `v_noabi::foo`
// via the `v_noabi` conversion function.
v_noabi::foo f = v_noabi::from_v1(v1::get_foo());

// error: from v1 to what? (ADL not supported.)
v_noabi::foo f = from_v1(v1::get_foo());

// error: conversion from `v_noabi::foo` to `v1::foo`
// must be explicit (one-way implicit conversions).
v1::foo f = v_noabi::get_foo();

// OK: explicit forward-compatibility via conversion function.
v1::foo f = v_noabi::to_v1(v_noabi::get_foo());

// OK: same as above, but found via ADL.
v1::foo f = to_v1(v_noabi::get_foo());

The (implicit) conversions will of course come with a performance cost, so relying on them too much is not recommended. However, this will faciliate incremental migration from vA to vB both in user code and in internal code.

@eramongodb eramongodb requested a review from kevinAlbs August 5, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants