CXX-3198 modernize ClangFormat configuration options #1306

eramongodb · 2024-12-30T16:26:32Z

Summary

Followup to #1299.

This PR proposes modifying several format options in the .clang-format configuration file to modernize and (hopefully) better fit our development practices/preferences. See ClangFormatStyleOptions (for 19.1.0, the version we currently use).

This PR is organized such that each option update is accompanied by corresponding changes to source code for comparison/reference. Particularly notable changes are to ColumnLimit, AlignAfterOpenBracket, and QualifierAlignment.

ColumnLimit: 100 -> 120

For consistency with the C Driver config and for arguably greater benefit to our C++ codebase by reducing instances of awkward line-wrapping (particularly for templates).

AlignAfterOpenBracket: Align -> AlwaysBreak

One of the major proposed changes in this PR. This is primarily motivated by the desire to remove (imo) excessive indentation of parameters and arguments, e.g.:

// Before:
bsoncxx::document::value owner =
    bsoncxx::builder::basic::make_document(kvp("a", std::int32_t{1}),  // "$numberInt": "1"
                                           kvp("b", std::int64_t{2}),  // "$numberLong": "2"
                                           kvp("c", binary)  // "$binary": { "$base64": "dGhyZWU=", "subType": 00 }
    );
bsoncxx::document::view doc = owner.view();

// After:
bsoncxx::document::value owner = bsoncxx::builder::basic::make_document(
    kvp("a", std::int32_t{1}),  // "$numberInt": "1"
    kvp("b", std::int64_t{2}),  // "$numberLong": "2"
    kvp("c", binary)            // "$binary": { "$base64": "dGhyZWU=", "subType": 00 }
);
bsoncxx::document::view doc = owner.view();

// Before:
constexpr friend bsoncxx::detail::strong_ordering tag_invoke(bsoncxx::detail::compare_three_way cmp,
                                                             basic_string_view left,
                                                             basic_string_view right) noexcept {
    return cmp(left.compare(right), 0);
}

// After:
constexpr friend bsoncxx::detail::strong_ordering tag_invoke(
    bsoncxx::detail::compare_three_way cmp, basic_string_view left, basic_string_view right) noexcept {
    return cmp(left.compare(right), 0);
}

There is a concern with this setting that a parameter list may be confused with the function body given the same indentation level, e.g.:

bsoncxx::v_noabi::document::value database::run_command(
    client_session const& session,
    bsoncxx::v_noabi::document::view_or_value command) {
    return _run_command(&session, command);
}

Although BlockIndent rather than AlwaysBreak would be preferable to avoid this issue (by forcing the closing brace to be on its own line at the base indentation level), this setting does not behave consistently due to only applying to braced initializer lists and parenthesis per the associated note in documentation (e.g. does not apply to template parameters). If sufficiently confusing, we can consider adjusting ContinuationIndentWidth to either 2 (half-indentation) or 8 (double-indentation):

// ContinuationIndentWidth: 2
bsoncxx::v_noabi::document::value database::run_command(
  client_session const& session,
  bsoncxx::v_noabi::document::view_or_value command) {
    return _run_command(&session, command);
}

// ContinuationIndentWidth: 8
bsoncxx::v_noabi::document::value database::run_command(
        client_session const& session,
        bsoncxx::v_noabi::document::view_or_value command) {
    return _run_command(&session, command);
}

AllowAllParametersOfDeclarationOnNextLine: true -> false

This change ensures whenever a parameter/argument list is broken after the opening brace, each parameter/argument is given its own line. This is to encourage better wrapping and slightly reduce the parameter list vs. function body confusion described above.

// Before:
constexpr friend bsoncxx::detail::strong_ordering tag_invoke(bsoncxx::detail::compare_three_way cmp,
                                                             basic_string_view left,
                                                             basic_string_view right) noexcept {
    return cmp(left.compare(right), 0);
}

// After:
constexpr friend bsoncxx::detail::strong_ordering
tag_invoke(bsoncxx::detail::compare_three_way cmp, basic_string_view left, basic_string_view right) noexcept {
    return cmp(left.compare(right), 0);
}

QualifierAlignment: Left -> Right

This change "reverts" the fixes applied in #1299 by switching from "west const" style (const T&) to "east const" style (T const&). This is motivated by better consistency between syntax and semantics across C++ code, e.g.:

using T = int*;

using A = T const; // int* const (OK: syntax matches semantics)
using B = const T; // int* const (!?: syntax does not match semantics)

Further motivations for this style is discussed in greater detail in C++ Templates: The Complete Guide, 2nd Edition, section xxxi: "Some Remarks About Programming Style". Note we've already let several instances of east const leak into new code, which were (temporarly) reverted by #1299.

SpacesBeforeTrailingComments: 2 -> 1

This change reduces the spaces count prior to // in closing namespace braces and similar cases (it is unclear why it was set to 2 to begin with).

#if !defined(MACRO_GUARD)
#define MACRO_GUARD

namespace example {

...

} // namespace example
 ^

#endif // !defined(MACRO_GUARD)
      ^

ShortNamespaceLines: 1 -> 0

This ensures the closing namespace brace is always accompanied by a trailing comment denoting the namespace being closed.

KeepEmptyLines.AtStartOfFile: true -> false

This trims extra newlines at the beginning of a file.

kevinAlbs

LGTM. I support the ColumnLimit increase.

eramongodb · 2025-01-02T20:23:38Z

error: declaration of ‘bsoncxx::v_noabi::types::bson_value::view bsoncxx::v_noabi::types::bson_value::value::view() const’ [-fpermissive]
     BSONCXX_ABI_EXPORT_CDECL(bson_value::view) view() const noexcept;
                                                             ^~~~~~~~
In file included from ../../../src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/types/bson_value/value.hpp:27,
                 from ../../../src/bsoncxx/lib/bsoncxx/v_noabi/bsoncxx/document/element.cpp:24:
../../../src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/types/bson_value/view.hpp:54:7: error: changes meaning of ‘view’ from ‘class bsoncxx::v_noabi::types::bson_value::view’ [-fpermissive]
 class view {
       ^~~~

It is unclear why this GCC error was triggered merely by a change from const view& to view const& in the parameter list for bsoncxx::v_noabi::types::bson_value::value's ctor, but qualifying the type with bson_value:: to disambiguate it from the member function suffices as a workaround (note: it should have been qualified to begin with). 🤔

adriandole

Looks good. I never liked the look of the highly indented arguments.

Suggestion: once merged, can you create a .git-blame-ignore-revs file containing this commit? Usage for GitLens and CLI described here.

eramongodb · 2025-01-03T15:18:00Z

@adriandole Due to GitHub not supporting fast-forward merges of PRs, the commit hash of the formatting commit can only be known after the PR has been merged. A followup PR is therefore necessary to add the .git-blame-ignore-revs file (e.g. as done for the C Driver with mongodb/mongo-c-driver#1546). I can add refs for both this and the prior PR after this PR is merged. 👍

(Edit: whoops, sorry for the unnecessary elaboration, overlooked the "once merged".)

ghost

I can't speak to the aesthetics of this change. It's subjective. I would make different choices, but I'm not aware of any stakeholder requirements that would support or refute these changes.

The 'const' movement seems especially risky; there's one fixup in here already, and I may have found another inconsistency in the interpretation of 'view' during review. The overall size of this diff is daunting: 789 files, 58kloc. I did my best to review it thoroughly for correctness.

I noticed some existing problems during the review: an unnecessary strlen and min, and more worrying a flawed RNG in the examples.

Using rand() like this even in a didactic context is troubling. (Maximum available entropy is going to be less than the key size even if rand() happens to have a secure entropy source) Additionally, change 0d649bf introduced a bug. The "% UINT8_MAX" pattern is not correct unless it's important to avoid 0xFF bytes for some reason I'm unaware of.

ghost · 2025-01-02T23:47:29Z

examples/mongocxx/explicit_encryption.cpp


 int EXAMPLES_CDECL main() {
    instance inst{};

    // This must be the same master key that was used to create
    // the encryption key; here, we use a random key as a placeholder.
    std::uint8_t key_storage[kKeyLength];
-    std::generate_n(key_storage, kKeyLength, []() {


Is this supposed to be excluding 0xFF bytes? Using rand() like this even as a didactic placeholder for a cryptographic rng seems problematic.

No, it shouldn't exclude 0xff. Good catch. Will address in a followup PR.

That being said, I do not believe the use of rand() is too concerning. Although it is demonstrating cryptography-related code, these generated bytes are not being used for any cryptographically secure purpose or application: it is just to satisfy the interface for sake of the example below (users do not ever manually generate their own key material like this; they use the key material returned from the server by a createKey operation).

These examples (which are entirely self-contained) have been largely left unchanged from their initial implementation in Sep 2020. 0d649bf introduced the current lambda expression + cast to remove a cast of std::rand to a function pointer (not permitted by stdlib specification) and implicit conversion from int -> std::uint8_t. Examples under examples/ (excluding the new examples/api/ examples) used to be embedded or referenced (in their entirety) in C++ Driver manual/documentation pages (not to be confused with Doxygen API documentation) but these have been removed over time. I expect these source files will also eventually be removed in favor of the new API doc examples, which take greater care to hide "placeholder" code and behavior which may distract from the purpose of the example, such as these random byte generators.

ghost · 2025-01-03T17:53:44Z

src/mongocxx/lib/mongocxx/v_noabi/mongocxx/exception/private/mongoc_error.hh

-                 msg,
-                 std::min(strlen(msg) + 1, static_cast<size_t>(BSON_ERROR_BUFFER_SIZE)));
+inline void set_bson_error_message(bson_error_t* error, char const* msg) {
+    bson_strncpy(error->message, msg, std::min(strlen(msg) + 1, static_cast<size_t>(BSON_ERROR_BUFFER_SIZE)));


This extra strlen and std::min isn't helping.

Good catch. This code seems to have been untouched (excluding recent formatting changes) since its introduction in Feb 2020. Will address in a followup PR.

ghost · 2025-01-04T00:39:31Z

src/bsoncxx/lib/bsoncxx/v_noabi/bsoncxx/types/bson_value/view.cpp

This "view::view(const view& rhs)" on line 55 didn't change. Might be another instance of the same confusion issue that commit 9a1f489 fixed one instance of.

The preprocessor directives preceeding the function definition seems to confuse ClangFormat. Grepping for const ([a-zA-Z_:]+)[&\*] reveal several more instances that appear to be missed due to preprocessor directives or being within strings and comments. These have been manually updated accordingly. It does not seem related to 9a1f489.

ghost · 2025-01-04T00:40:32Z

examples/mongocxx/automatic_client_side_field_level_encryption.cpp

-    });
-    bsoncxx::types::b_binary local_master_key{
-        bsoncxx::binary_sub_type::k_binary, kKeyLength, key_storage};
+    std::generate_n(key_storage, kKeyLength, []() { return static_cast<std::uint8_t>(std::rand() % UINT8_MAX); });


Another std::rand() % UINT8_MAX

ghost · 2025-01-04T00:41:22Z

examples/mongocxx/explicit_encryption_auto_decryption.cpp

-    });
-    bsoncxx::types::b_binary local_master_key{
-        bsoncxx::binary_sub_type::k_binary, kKeyLength, key_storage};
+    std::generate_n(key_storage, kKeyLength, []() { return static_cast<std::uint8_t>(std::rand() % UINT8_MAX); });


Another std::rand() % UINT8_MAX

ghost · 2025-01-04T00:42:18Z

examples/mongocxx/server_side_field_level_encryption_enforcement.cpp

-    });
-    bsoncxx::types::b_binary local_master_key{
-        bsoncxx::binary_sub_type::k_binary, kKeyLength, key_storage};
+    std::generate_n(key_storage, kKeyLength, []() { return static_cast<std::uint8_t>(std::rand() % UINT8_MAX); });


And this one

* Remove unnecessary strlen+min on call to bson_strncpy * Use pre-generated key material for examples instead of rand()

eramongodb added 7 commits December 30, 2024 09:07

format: set ColumnLimit: 120

5f2dbbd

format: set AlignAfterOpenBracket: AlwaysBreak

9705610

format: set AllowAllParametersOfDeclarationOnNextLine: false

b6c757a

format: set QualifierAlignment: Right

c951ae4

format: set SpacesBeforeTrailingComments: 1

a9a6aa1

format: set ShortNamespaceLines: 0

b61de43

format: set KeepEmptyLines.AtStartOfFile: false

b3f62a8

eramongodb requested a review from kevinAlbs December 30, 2024 16:26

eramongodb self-assigned this Dec 30, 2024

kevinAlbs requested review from vector-of-bool, a user, adriandole and kevinAlbs and removed request for kevinAlbs and vector-of-bool December 30, 2024 16:43

kevinAlbs approved these changes Dec 30, 2024

View reviewed changes

eramongodb added 2 commits January 2, 2025 14:18

Address GCC -fpermissive error due to view declaration conflict

9a1f489

Merge remote-tracking branch 'upstream/master' into cxx-3198

8316ad2

adriandole approved these changes Jan 3, 2025

View reviewed changes

ghost approved these changes Jan 4, 2025

View reviewed changes

eramongodb added 2 commits January 6, 2025 10:02

Merge remote-tracking branch 'upstream/master' into cxx-3198

aedfa8a

Manually fix qualifier alignments missed by ClangFormat

156b5e8

eramongodb merged commit 2f53a56 into mongodb:master Jan 6, 2025
11 of 14 checks passed

eramongodb deleted the cxx-3198 branch January 6, 2025 16:30

eramongodb mentioned this pull request Jan 6, 2025

CXX-3198 Miscellaneous improvements to address feedback in #1306 #1310

Merged

eramongodb added a commit that referenced this pull request Jan 7, 2025

CXX-3198 Miscellaneous improvements to address feedback in #1306 (#1310)

59a8c10

* Remove unnecessary strlen+min on call to bson_strncpy * Use pre-generated key material for examples instead of rand()

eramongodb mentioned this pull request Jan 15, 2025

Add git blame ignore refs for f1ad6b63 and 2f53a561 #1320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CXX-3198 modernize ClangFormat configuration options #1306

CXX-3198 modernize ClangFormat configuration options #1306

Uh oh!

eramongodb commented Dec 30, 2024 •

edited

Loading

Uh oh!

kevinAlbs left a comment

Uh oh!

eramongodb commented Jan 2, 2025 •

edited

Loading

Uh oh!

adriandole left a comment •

edited

Loading

Uh oh!

eramongodb commented Jan 3, 2025 •

edited

Loading

Uh oh!

ghost left a comment

Uh oh!

ghost Jan 2, 2025

Uh oh!

eramongodb Jan 6, 2025

Uh oh!

ghost Jan 3, 2025

Uh oh!

eramongodb Jan 6, 2025

Uh oh!

ghost Jan 4, 2025

Uh oh!

eramongodb Jan 6, 2025 •

edited

Loading

Uh oh!

ghost Jan 4, 2025

Uh oh!

ghost Jan 4, 2025

Uh oh!

ghost Jan 4, 2025

Uh oh!

Uh oh!

Uh oh!

CXX-3198 modernize ClangFormat configuration options #1306

CXX-3198 modernize ClangFormat configuration options #1306

Uh oh!

Conversation

eramongodb commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

ColumnLimit: 100 -> 120

AlignAfterOpenBracket: Align -> AlwaysBreak

AllowAllParametersOfDeclarationOnNextLine: true -> false

QualifierAlignment: Left -> Right

SpacesBeforeTrailingComments: 2 -> 1

ShortNamespaceLines: 1 -> 0

KeepEmptyLines.AtStartOfFile: true -> false

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

eramongodb commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriandole left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eramongodb commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eramongodb Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eramongodb commented Dec 30, 2024 •

edited

Loading

eramongodb commented Jan 2, 2025 •

edited

Loading

adriandole left a comment •

edited

Loading

eramongodb commented Jan 3, 2025 •

edited

Loading

eramongodb Jan 6, 2025 •

edited

Loading