clang-format for C++

[bernardnormier]

The goal is have C++ format that is close to:

• the IceRPC C# style
• the existing Ice C++ style
• the cppreference style

And of course that looks nice.

Here is my initial attempt. Give it a try and improve it!

[pepone]

Would be good to apply the format to see the actual changes.

[pepone]

I run clang-format -i and changes look overall fine, we should wait until there isn't any large outgoing PRs to format the code.

[externl]

Looks good!!!

[externl]

I run clang-format -i and changes look overall fine, we should wait until there isn't any large outgoing PRs to format the code.

I agree, would be nice to get this in soon though.

[bernardnormier]

I applied it to the C++ code so that you can see it on your favorite code. I am not proposing to merge this PR as-is - we need to coordinate to avoid big merge conflicts.

I had to disable include sorting as it broke the build.

We also need to write a separate clang format for the Objective-C/C++ code, including the header files.

[InsertCreativityHere]

I think it's an improvement in general.
But there were a few things that looked weird in the 20 or so files I skimmed.

[InsertCreativityHere]

Even though this does fit within a 120 column line,
I think it's more readable to keep all but the trivialist of functions on multiple lines.

If there's no way to tweak this, so be it.

[InsertCreativityHere]

There's a configuration option for this:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortfunctionsonasingleline
But it doesn't let you change it's behavior based on the length of the function body...
There are some other options for it that I think are worth looking at.

Personally, I'd prefer Empty or Inline... it's hard to tell what the values are by default.

[bernardnormier]

I prefer this kind of code on one line.

[InsertCreativityHere]

Should we add an exception for all the Bison generated files (so the formatter ignores them)?
A) No sane person will ever read this code
B) Regenerating the parsers will be slightly more painful if we need to fix the formatting of them

[bernardnormier]

This would be desirable. I didn't research how to do it.

[InsertCreativityHere]

What's everyone's thoughts on this trailing/leading whitespace around namespaces?

I've mostly only seen:

namespace Foo
{

stuff

}

and

namespace Foo
{
    stuff
}

So, what we're doing now feels kind of weird:

namespace Foo
{

    stuff

}

I personally think the 2nd option looks the best, and it's also what cppreference uses:
https://en.cppreference.com/w/cpp/language/namespace

[InsertCreativityHere]

Doesn't look there's a way to enforce this with the formatter... sad.
Could probably still be fixed with a regex if everyone else agrees it looks weird.

[pepone]

I prefer 2nd option too

[bernardnormier]

Yes, we should remove these blank lines. They were not inserted by the formatter, it's just a consequence of our current zero-indent in namespace formatting.

[pepone]

What about adding:

diff --git a/.clang-format b/.clang-format
index 4a4a7e7ad9..b4a36224fb 100644
--- a/.clang-format
+++ b/.clang-format
@@ -20,3 +20,9 @@ PointerAlignment: Left
 SortIncludes: Never
 TabWidth: 4
 UseTab: Never
+
+AlignAfterOpenBracket: AlwaysBreak
+AllowAllArgumentsOnNextLine: false
+AllowAllParametersOfDeclarationOnNextLine: false
+AllowAllConstructorInitializersOnNextLine: false

I think it would be more consistent with our IceRCP C# style, for example:

diff --git a/cpp/src/Ice/ACM.cpp b/cpp/src/Ice/ACM.cpp
index dea228841d..bd27c19c28 100644
--- a/cpp/src/Ice/ACM.cpp
+++ b/cpp/src/Ice/ACM.cpp
@@ -20,10 +20,11 @@ IceInternal::ACMConfig::ACMConfig(bool server)
 {
 }
 
-IceInternal::ACMConfig::ACMConfig(const Ice::PropertiesPtr& p,
-                                  const Ice::LoggerPtr& l,
-                                  const string& prefix,
-                                  const ACMConfig& dflt)
+IceInternal::ACMConfig::ACMConfig(
+    const Ice::PropertiesPtr& p,
+    const Ice::LoggerPtr& l,
+    const string& prefix,
+    const ACMConfig& dflt)
 {
     string timeoutProperty;
     if ((prefix == "Ice.ACM.Client" || prefix == "Ice.ACM.Server") && p->getProperty(prefix + ".Timeout").empty())
@@ -128,8 +129,9 @@ IceInternal::FactoryACMMonitor::add(const ConnectionIPtr& connection)
     if (_connections.empty())
     {
         _connections.insert(connection);
-        _instance->timer()->scheduleRepeated(shared_from_this(),
-                                             chrono::duration_cast<chrono::nanoseconds>(_config.timeout) / 2);
+        _instance->timer()->scheduleRepeated(
+            shared_from_this(),
+            chrono::duration_cast<chrono::nanoseconds>(_config.timeout) / 2);
     }
     else
     {
@@ -158,9 +160,10 @@ IceInternal::FactoryACMMonitor::reap(const ConnectionIPtr& connection)
 }
 
 ACMMonitorPtr
-IceInternal::FactoryACMMonitor::acm(const optional<int>& timeout,
-                                    const optional<Ice::ACMClose>& close,
-                                    const optional<Ice::ACMHeartbeat>& heartbeat)
+IceInternal::FactoryACMMonitor::acm(
+    const optional<int>& timeout,
+    const optional<Ice::ACMClose>& close,
+    const optional<Ice::ACMHeartbeat>& heartbeat)
 {

[bernardnormier]

What about adding:

Applied.

[bernardnormier]

Replaced by #1916.