C++ SDK should follow Google Style

[devjgm]

The C++ SDK should following Google's C++ style guide and generally recommended coding conventions, such as https://abseil.io/tips/

Concrete next steps that I recommend:

1. Fix all formatting of .cc and .h files with clang-format -style=Google .... Ideally, have a CI build that verifies new PRs are properly formatted.

2. Misc. C++ code cleanups, like more use of unique_ptr (I think there are several leaks of grpc::ClientContext currently), remove unnecessary std::string copies, etc.

If changes like this sound acceptable to the project owners, I might be able to either (a) make these changes, or (b) review these changes if someone else wants to make them.

[roberthbailey]

/cc @dsazonoff

I'm partial to using Google's C++ style guide, but I would be happy if we specified any style guide and stuck with it (using one even if it isn't perfect is better than not using one). And if we want a style guide, picking one that has auto-formatting seems like a big win.

[roberthbailey]

As per the second point, the C++ SDK docs say:

The C++ SDK is specifically designed to be as simple as possible, and deliberately doesn’t include any kind of singleton management, or threading/asynchronous processing to allow developers to manage these aspects as they deem appropriate for their system.

but I think it would be nice to not leak resources even if we don't provide sophisticated threading support.

[markmandel]

I personally don't have strong opinions here regarding C++, I'm happy to defer to those that do and have actual real experience with C++ (which I definitely do not have!)

[devjgm]

@roberthbailey thanks. I did see the C++ SDK docs. Keeping the implementation simple SGTM. I'm not suggesting anything to the contrary. I suggest fixing the memory leaks, but also there are generally accepted C++ best practices (e.g., http://abseil.io/tips, and the C++ Core Guidelines) that I think the code should adhere to as well.

[roberthbailey]

Adhering to best practices sgtm. I just want to make sure that we don't force any particular usage pattern upon the client.

@dsazonoff is in a different timezone so I think we should give him a chance to chime in before settling anything since he's been working on the C++ SDK.

[devjgm]

SGTM.

Following the Google Style guide seems like it might be fairly uncontroversial, so I created #716, which was autogenerated by clang-format -style=Google .... I only formatted the sdk.{h,cc} files, not the files generated by protoc/grpc.

[dsazonoff]

Ideally, have a CI build that verifies new PRs are properly formatted.
I think that it is good idea to have this check as a build step. It can be implemented much easier then commit hooks etc.

Your PR is OK, I'll add a build step to verify codestyle. The only one difference - we will use C++14 (default in codestyle is C++11).

C++ code cleanups, like more use of unique_ptr (I think there are several leaks of grpc::ClientContext currently), remove unnecessary std::string copies, etc.

I'll fix it.

So choice is (a) make these changes, and I'll add you to reviewers.

[roberthbailey]

I think that this was finished by #855. @devjgm did you have any follow up actions. I can say from personal experience that if you forget to clang-format the .cc / .h files the presubmits will let you know.

[dsazonoff]

FYI, it is possible to specify cmake <...> -DAGONES_CLANG_FORMAT=ON option to force code reformatting as a pre-build step for local development. I didn't put it in documentation, because it's a very rare case when it may be necessary. Usually IDE handles code formatting.

[roberthbailey]

Thanks @dsazonoff for that hint. I've just been manually formatting the code as I modify the C++ files since my IDE isn't configured to do it correctly.

As per my last comment, I'm going to close this issue. Feel free to reopen if there is any remaining work to be done.