Coding Standards: Switch to using in-class initializers instead of initializers in constructors

From discussion in #825. We should probably make a point to move existing code over to this style. @adiviness might have ideas on the best way to go about this.

The relevant discussion is copied below:

dlong11 5 days ago

@zadjii-msft Some of these variables should be class initialized. C.48: Prefer in-class initializers to member initializers in constructors for constant initializers

zadjii-msft 3 days ago Author Member

Considering this is the style that we're consistently using throughout the rest of the codebase, I'm going to leave these.

Seems kinda odd that we've been converting things to use this style when this isn't the standard, but I'm not the standards guru here - @adiviness is

adiviness 2 days ago Member

The reasoning for doing all initialization in the constructor is to prevent splitting the initialization code across two files, it's a little easier to read when it's all done in the constructor. Ideally we could get around the chance of missing initialization of a field with a compiler warning like what clang has. This may be something we can revisit one the static analysis tools are in place.

dlong11 2 days ago

@zadjii-msft @adiviness @DHowett-MSFT @binarycrusader I wasn't sure who this comment should go to so I've added everyone I have interacted with.

From an outsiders point of view - It is very difficult to review code when you don't know what the coding standards are. I have seen that you follow the Cpp Core Guidelines but only in certain areas? Maybe this comes from the fact that you have a distinction from "old" code vs "new" code. I believe it may come from the clause - If it's inserting something into the existing classes/functions, try to follow the existing style as closely as possible. I understand the reasoning of this but this can be a real trap when you are dealing with coding standards.

This essentially means you have two coding standards and now you must know when to apply each. This may be easy for someone who is on the "team" but this can be a deal breaker for the outside community. When and how will this code get updated to the new standards? Why would we continually add code debt? Cleaning this code up in the past may have been impossible due to the team size, but now you have a community that can/will do this if they know the rules.

This is a good example -

The reasoning for doing all initialization in the constructor is to prevent splitting the initialization code across two files, it's a little easier to read when it's all done in the constructor.

This is contradictory to C.48: Prefer in-class initializers to member initializers in constructors for constant initializers

I am fine with either approach, but to effectively review this code we need to know which set of rules we are following.

I don't want this to come off as ranting. This is purely feedback and not meant to cast stones at anyone! I am just trying to figure out the rules and expectations. (I'm sure others are as well) Also, I am not trying to push for one standard vs another. Since this project has been newly opened up, it is entirely possible that you guys are still figuring this out. I can understand that.

I thought it might be helpful to give you an outsiders point of view.

zadjii-msft a day ago Author Member

This is a very valid point, and I want to make sure you don't feel like we are just dismissing this opinion. Updating our code to modern standards has been a long and arduous journey, but we're definitely not all there yet (evidently). I think where we're mostly coming from is a very Windows-centric mindset, where we have a huge codebase with varying styles across all of it. In Windows, the guidance* has been to match the existing style when writing code in an existing codebase, and try to conform to the standard when writing a new feature areas.

I definitely hear you that this is a major pain points for external contributors, and something that we really need to work with the community on going forward. There's #890, which is probably a good thread to copy most of this commentary into.

adiviness a day ago Member

Typically how we've done stylistic things in the past is that new code gets written in whatever we considered the best of coding standards/styling at the time, but we also don't know everything about C++ :) I'm actively looking into getting code formatting/linting/static analysis/etc. tools set up for the project. For something like the initializers I'd expect that we'd enable whatever check that makes a tool yell at us about them and then we'd fix all of the instances in one fell swoop. Although I'm not sure if it's possible for a tool to find instances of this particular coding guideline quite yet.

We do want to more closely follow the coding standards we've referenced, this may be something worth opening an issue for so switching the initializers could be tracked.

dlong11 17 hours ago

@zadjii-msft Thanks for the reply.

and I want to make sure you don't feel like we are just dismissing this opinion.

I don't feel like anyone is dismissing my opinion. You guys have been great. I understand that we are in forming/storming stage. All good.

Updating our code to modern standards has been a long and arduous journey, but we're definitely not all there yet (evidently). I think where we're mostly coming from is a very Windows-centric mindset, where we have a huge codebase with varying styles across all of it.

Too funny. I am in the same boat. I have been a working a large legacy windows-centric code base for a long time. A few years ago we decided to go cross platform and start to update to Modern C++. So we were in the same boat. It has been a long road and obviously when you have millions of lines of code you can't clean it all up at once. We started with a similar set of rules like you. "keep the code consistent, ....." What we found was doing this is almost impossible. It is really hard to say "follow these guidelines for this code" and "follow these for this code".

In a nutshell this is what helped us get there.

Have a style guide and follow it for all new and modified code. The more detail the better.
I mean really follow it. :) This sounds funny but until everyone commits to it and understands it you will have issues. All devs/reviewers need to understand the Core guidelines (or whatever your coding standards are) - this takes some time to get there but it is doable.

Compile with all Major compilers - Clang has great sanitizers, better warnings, etc...

Use clang-format! This will save a huge amount of time. No more - oh you have an extra line here, you have the braces in the wrong spot, etc... Also, be OK if it is not exactly the same as what you do now. Believe me, it is worth it.

Setup external tools like the CppCoreGuilines analyzer / static analyzer - this sounds great but with a large code base, it is difficult to run the analyzer and fix them all. In this repo you may be able to do that. The terminal codebase isn't that large.

Document - Contributing.md, style guide, Do's / Dont's, etc. If everyone knows what the rules are then everyone will be more productive. Also realize the community doesn't have the years of backstory that msft devs have. (this is easy to forget) Doing a good job of this will pay huge dividends.

There's #890, which is probably a good thread to copy most of this commentary into.

I opened this. 👍 For this repo to be successful this stuff is a must.

You guys seem to have a good start on this and @adiviness has a lot of this on his list. So you guys are on the right track. I'm looking forward to watching this repo mature.