I'd love to have a better visual separation between c'tor member initializer lists and c'tor code. E.g.:

Current:

Channel_Impl_13::Channel_Impl_13(const std::shared_ptr<Callbacks>& callbacks,
                                 const std::shared_ptr<Session_Manager>& session_manager,
                                 const std::shared_ptr<Credentials_Manager>& credentials_manager,
                                 const std::shared_ptr<RandomNumberGenerator>& rng,
                                 const std::shared_ptr<const Policy>& policy,
                                 bool is_server) :
   m_side(is_server ? Connection_Side::Server : Connection_Side::Client),
   m_callbacks(callbacks),
   m_session_manager(session_manager),
   m_credentials_manager(credentials_manager),
   m_rng(rng),
   m_policy(policy),
   m_record_layer(m_side),
   m_handshake_layer(m_side),
   m_can_read(true),
   m_can_write(true),
   m_opportunistic_key_update(false),
   m_first_message_sent(false),
   m_first_message_received(false) {
   BOTAN_ASSERT_NONNULL(m_callbacks);
   BOTAN_ASSERT_NONNULL(m_session_manager);
   BOTAN_ASSERT_NONNULL(m_credentials_manager);
   BOTAN_ASSERT_NONNULL(m_rng);
   BOTAN_ASSERT_NONNULL(m_policy);
}

Ideas

ConstructorInitializerIndentWidth: 6:

Channel_Impl_13::Channel_Impl_13(/* ... */) :
      /* ... */
      m_opportunistic_key_update(false),
      m_first_message_sent(false),
      m_first_message_received(false) {
   BOTAN_ASSERT_NONNULL(m_callbacks);
   BOTAN_ASSERT_NONNULL(m_session_manager);
   /* ... */
}

BreakConstructorInitializers: BeforeComma:

Channel_Impl_13::Channel_Impl_13(/* ... */)
   : m_side(is_server ? Connection_Side::Server : Connection_Side::Client)
   /* ... */
   , m_opportunistic_key_update(false)
   , m_first_message_sent(false)
   , m_first_message_received(false) {
   BOTAN_ASSERT_NONNULL(m_callbacks);
   BOTAN_ASSERT_NONNULL(m_session_manager);
   /* ... */
}

OK everything seems to work. Last call to bikeshed on the configuration. If nothing is raised I will merge ~ Saturday.

The Windows amalgamation does not work.

Coverage: 92.227% (+0.5%) from 91.723% when pulling 3f87e1e on jack/clang-format into 289cfb1 on master.

We should probably all agree on a concrete clang-format version to use, otherwise the CI validation could become annoying.

clang-format being a developer tool, I think it should be as new as possible. E.g. support for C++20 concepts is basically non-extistent before clang-format 15 and leaves a lot to be desired in clang-format 16. 😢

My suggestion: Let's stick with 15, as this is available in Ubuntu 22.04.

Particularly, for the concepts.h I would probably disable clang-format until better support is available. The resulting formattting is quite "impressionistic" right now.

Suggestion: Add requires directives available in clang-format 15

RequiresClausePosition: OwnLine
IndentRequiresClause: true

Here's are a few wishes:

Visual separation of declaration and implementation

If the parameter list is floating to the next line (e.g. due to long function names), its hard to visually see where the function implementation starts:

std::vector<X509_Certificate> search_cert_stores(
   const _CRYPTOAPI_BLOB& blob,
   const DWORD& find_type,
   std::function<bool(const std::vector<X509_Certificate>& certs, const X509_Certificate& cert)>
      filter,
   bool return_on_first_found) {
   std::vector<X509_Certificate> certs;
   for(const auto store_name : cert_store_names) {
      // [...]
   }

   return certs;
}

Suggestion 1: Indent "continuation lines" further

ContinuationIndentWidth: 6

std::vector<X509_Certificate> search_cert_stores(
      const _CRYPTOAPI_BLOB& blob,
      const DWORD& find_type,
      std::function<bool(const std::vector<X509_Certificate>& certs, const X509_Certificate& cert)>
         filter,
      bool return_on_first_found) {
   std::vector<X509_Certificate> certs;
   for(const auto store_name : cert_store_names) {
      // [...]
   }

   return certs;
}

Suggestion 2: Add newline before braces of declarations

BreakBeforeBraces: Custom
BraceWrapping:
  AfterFunction: true
  AfterStruct: true
  AfterClass: true
  AfterEnum: true
  AfterUnion: true

std::vector<X509_Certificate> search_cert_stores(
   const _CRYPTOAPI_BLOB& blob,
   const DWORD& find_type,
   std::function<bool(const std::vector<X509_Certificate>& certs, const X509_Certificate& cert)>
      filter,
   bool return_on_first_found)
{
   std::vector<X509_Certificate> certs;
   for(const auto store_name : cert_store_names) {
      // [...]
   }

   return certs;
}

Suggestion 3: Do both of the above

That's my preference, as it emphasizes this visual separation the most.

std::vector<X509_Certificate> search_cert_stores(
      const _CRYPTOAPI_BLOB& blob,
      const DWORD& find_type,
      std::function<bool(const std::vector<X509_Certificate>& certs, const X509_Certificate& cert)>
         filter,
      bool return_on_first_found)
{
   std::vector<X509_Certificate> certs;
   for(const auto store_name : cert_store_names) {
      // [...]
   }

   return certs;
}

One minor thing: FixNamespaceComments: true is a nice convenience as those closing braces tend to appear to be rogue most of the time. You disabled it explicitly, though. So I'm wondering if you had a reason for that.

Suggestion 1: Indent "continuation lines" further

This seems fine but tbh I would think any time a parameter type is so long that it can't fit on a single line, it should be a typedef

Suggestion 2: Add newline before braces of declarations

TBH I find that just as verbose as the current style, but harder to parse

FixNamespaceComments: true

I find these to be just kind of noisy without much benefit, but maybe that's just because emacs always shows what the matching start of the block is anyway (though I would think any other editor would do the same??). Not totally adverse to it just wondering what I am missing.

My suggestion: Let's stick with 15, as this is available in Ubuntu 22.04.

Right now the formatter script refuses to run on anything other than clang-format 15. We'll have to loosen this eventually I imagine, but it seems not improbable that clang-format 16 (or 17 or ...) will even with the same configuration format files differently, which would break our formatter checks, so I started out with the most conservative option and we can figure it out as we go.

Suggestion: Add requires directives available in clang-format 15

I'll add these

Also, as a general comment - any formatter is not going to do the right thing some proportion of the time. Even in Rust where the language and formatter are co-evolved, rustfmt does stupid things all the time. Certainly clang-format is doing all kinds of bad things here, many of which I cannot find any configuration option to make it stop. For instance why the hell does it format an 18 element array onto 18 lines in blowfish.cpp???? I hate it.

Which is one reason I have avoided using a formatter in the past, because with careful hand formatting the result is always cleaner. But less consistent, especially as more people contribute, so this approach does not scale so well. The only real fix is to accept that the initial automatic formatting is, no matter what configuration we use, not going to be ideal, and that for best readability it will be required to reformulate some code.

New code, being written with the formatter in place, will more naturally co-evolve with the limitations of the formatter.

Suggestion 1: Indent "continuation lines" further

This seems fine but tbh I would think any time a parameter type is so long that it can't fit on a single line, it should be a typedef

I couldn't agree more on the typedef. And admittedly, I have a hard time finding a bunch of other examples to back this argument. Let's leave as is then. 👍

FixNamespaceComments: true

Not totally adverse to it just wondering what I am missing.

Typically, namespace braces aren't following the usual indentation rules. Therefore, I occasionally confuse them with closing braces of other scopes. Particularly, when using nested namespaces (like anonymous or detail) that don't end at the very end of the file. Editors help, but when browsing code on GitHub we don't have this. Personally, I like the extra visual assurance that those stray braces are indeed for namespaces (and for which one exactly).

The only real fix is to accept that the initial automatic formatting is, no matter what configuration we use, not going to be ideal, and that for best readability it will be required to reformulate some code.

Let's fix it! 😄