Adding clang-format

[mredd]

This pull request adds a clang-format definition that tries to limit the amount of changes to the project. However, there are some inconsistencies in the current formatting that get reformatted by this pull request.

Feel free to pull or discard.

[daniele77]

Thank you very much @mredd
I'm not a big fan of clang-format because I never found a .clang-format file that fully satisfied my taste.
I'd like very much to use a tool to get consistency in indentation and spacing; however, in some cases, I find manually formatted code more expressive and readable. E.g.:

    return Proxy::Command(
        Method::post,
        "/ari/channels/"+id+"/move?"
        "app=" + app +
        ( appArgs.empty() ? "" : "&appArgs=" + appArgs ),
        client
    );

VS

    return Proxy::Command(
        Method::post,
        "/ari/channels/" + id +
            "/move?"
            "app=" +
            app + (appArgs.empty() ? "" : "&appArgs=" + appArgs),
        client);

In that case, a tool cannot possibly formatting the code properly, because clang-format cannot understand that
app + (appArgs.empty() ? "" : "&appArgs=" + appArgs)
is the value of app parameter.

I believe that some kinds of formatting are additional information with a semantic value the developer put in the sources just as he does with comments, it's not redundant with the code, so a tool cannot possibly calculate it by just reading the code.
E.g., horizontal spacing between some uncorrelated statements, new lines added to group parameters are part of the semantic information the developer adds to its code and it cannot be deduced by the code itself.
Of course, using spaces instead of tabs, indenting, adding or removing a space before braces are instead things that should be done automatically because they don't have additional meaning.
Unfortunately, clang-format works by parsing everything and re-writing the whole thing reformatted. You cannot only enable a subset of formatting rules and leave unaltered the remaining code.
(Maybe I'm gonna write a blog post on this subject :-) )

However your formatting file is really good, you did a good job trying to keep the formatting I'm used to. Maybe I could run clang-format once on my locale codebase and then review the differences to keep only the changes that don't break readability.

[mredd]

Yeah, I get your point with the URL formatting. Clang-format requires you to make your intent explizit, otherwise it assumes you don't care about the formatting and it harmonizes it according to the project rules.

This is how you tell clang-format to stay away from your linebreaks (as long as you don't violate the column limit):

return Proxy::Command(
    Method::post,
    "/ari/channels/"+id+"/move?" + //
    "app=" + app + //
    ( appArgs.empty() ? "" : "&appArgs=" + appArgs ), //
    client
);

which will be converted into this:

return Proxy::Command(
    Method::post,
    "/ari/channels/" + id + "/move?" +                  //
        "app=" + app +                                  //
        (appArgs.empty() ? "" : "&appArgs=" + appArgs), //
    client);

Noticed that "/move?" and "app=" is no longer a multiline string but two strings? This is the little performance impact you have to take. But as we are throwing strings around like nothing in HTTP and JSON, I think that this performance impact is just academic.

And when you want it to absolutely stay away of your ASCII art, there is always clang-format off and clang-format on as a last resort. But I personally don't like them because clang-format does not even harmonize the spaces and indentation and all that simple stuff in these blocks. It just parses them to understand where for instance a function ends to be back on track with indentation after clang-format on. The empty comments at line endings usually do the trick for 99% of the formatting.

I really like this because it makes it explicit where a developer cared about a specific formatting. This signals to the future reader that there is something worth paying attention to. The same works for function parameters and arguments, unless you use lambdas as direct arguments.

For instance, a sloppy

Proxy& Move( //
  const std::string& app, //
const std::string& appArgs = {}) const //
{
    OtherFunction( //
      arg1, //
      arg2, //
      arg3); //
}

gets cleaned up to

Proxy& Move(                               //
    const std::string& app,                //
    const std::string& appArgs = {}) const //
{
    OtherFunction( //
        arg1,      //
        arg2,      //
        arg3);     //
}

Regarding the grouping by blank lines, these get preserved by clang-format. It only harmonizes them to a single blank line where the developer put multiple in. When you really want to have a huge vertical distance, you should probably put a comment in on why the following block is special anyway. Where it messes with your blanks is around access modifier keywords (public, protected, private). But within a class/struct/function body you are free to group however you want. Check the AriModel for instance:

class AriModel
{
public:
    using ChannelPtr = std::shared_ptr<Channel>;

    explicit AriModel(Client& c) : client(c) { Subscribe(); }

    AriModel(const AriModel&) = delete;
    AriModel(const AriModel&&) = delete;
    AriModel& operator=(const AriModel&) = delete;
    AriModel& operator=(const AriModel&&) = delete;

    using ChHandler = std::function<void(ChannelPtr)>;
    using ChVarSetHandler =
        std::function<void(ChannelPtr, const std::string&, const std::string&)>;
    using ChDtmfHandler = std::function<void(ChannelPtr, const std::string&)>;
    using PlaybackHandler = std::function<void(const Playback&)>;
    using StasisStartedHandler = std::function<void(ChannelPtr, bool external)>;

I will provide an updated PR for you to review and decide on.

[daniele77]

OK @mredd , you almost convinced me :-)
Just a couple of thing:

1. the tests fails! (see the github CI output, please)
2. I'd like the ColumnLimit a little bigger then 90
3. I see that around line 55 of basicauth.h the tool made a refactory! Really?
4. I'd like to always have a line break between template parameters and class name eg. template<class Dummy> struct RoleBase split in two lines)

[mredd]

OK @mredd , you almost convinced me :-)
Just a couple of thing:

1. the tests fails! (see the github CI output, please)

Sorry, I didn't spot that error in the Github CI. The tests are failing at all on my local machine. See #40
Fixed now.

2. I'd like the ColumnLimit a little bigger then 90

Do you have any specific column limit in mind? How about 120?

3. I see that around line 55 of basicauth.h the tool made a refactory! Really?

Where do you see a refactoring there? Can you be a bit more specific? Don't get confused by the Github diff viewer. It bails out too early and doesn't get back on track. When you ignore the whitespace changes, this is what's left: (with the old 90 columns limit)

diff --git a/aricpp/basicauth.h b/aricpp/basicauth.h
index cd5f9dd..f0c8356 100644
--- a/aricpp/basicauth.h
+++ b/aricpp/basicauth.h
@@ -30,19 +30,17 @@
  * DEALINGS IN THE SOFTWARE.
  ******************************************************************************/
 
-
 #ifndef ARICPP_BASICAUTH_H_
 #define ARICPP_BASICAUTH_H_
 
-#include <string>
 #include <array>
+#include <string>
 
 namespace aricpp
 {
 namespace
 {
-    static const std::string base64_chars =
-        "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+static const std::string base64_chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
                                         "abcdefghijklmnopqrstuvwxyz"
                                         "0123456789+/";
 
@@ -59,8 +57,10 @@ namespace
         if (i == 3)
         {
             char_array_4[0] = (char_array_3[0] & 0xfc) >> 2;
-                char_array_4[1] = ((char_array_3[0] & 0x03) << 4) + ((char_array_3[1] & 0xf0) >> 4);
-                char_array_4[2] = ((char_array_3[1] & 0x0f) << 2) + ((char_array_3[2] & 0xc0) >> 6);
+            char_array_4[1] =
+                ((char_array_3[0] & 0x03) << 4) + ((char_array_3[1] & 0xf0) >> 4);
+            char_array_4[2] =
+                ((char_array_3[1] & 0x0f) << 2) + ((char_array_3[2] & 0xc0) >> 6);
             char_array_4[3] = char_array_3[2] & 0x3f;
 
             for (auto x : char_array_4)
@@ -76,8 +76,10 @@ namespace
             char_array_3[j] = '\0';
 
         char_array_4[0] = (char_array_3[0] & 0xfc) >> 2;
-            char_array_4[1] = ((char_array_3[0] & 0x03) << 4) + ((char_array_3[1] & 0xf0) >> 4);
-            char_array_4[2] = ((char_array_3[1] & 0x0f) << 2) + ((char_array_3[2] & 0xc0) >> 6);
+        char_array_4[1] =
+            ((char_array_3[0] & 0x03) << 4) + ((char_array_3[1] & 0xf0) >> 4);
+        char_array_4[2] =
+            ((char_array_3[1] & 0x0f) << 2) + ((char_array_3[2] & 0xc0) >> 6);
         char_array_4[3] = char_array_3[2] & 0x3f;
 
         for (auto j = 0; (j < i + 1); ++j)

4. I'd like to always have a line break between template parameters and class name eg. template<class Dummy> struct RoleBase split in two lines)

Yeah, I saw that. I decided against it because it looks ugly when you have lots of forward declarations. With the latest change, you have it back. :-)