Add code style/ naming guidelines

Add explicit guidelines as a reference to avoid ambiguities.

changed milestone to %0.19.00

added Component - Core Component - Fatras Component - Plugins Needs Decision labels

Rough proposal based on the most common style in the existing code base:

// CamelCase (w/ upper case initial) for all custom types, including typedefs for fundamental types
// No `..._t` or `..._type` here
class ComplicatedNamedClass;
struct AnotherStruct;
enum EnumType;
using SemanticName = int32;
// Avoid abbreviations; if still used, only use upper case for the first letter sincet the
// abbreviation is semantically a single word, e.g. VertexSomething could be
struct VtxSomething; // not VTXSomething.
// This is bad example since the name should not be abbreviated here anyways.

// snake_case_t for template parameter types
// To clarify that these are dynamic types and to avoid naming collisions
// when reexporting the type
template<typename large_vector_t>
struct Algorithm {
  // make the template type publicly available
  using LargeVector = large_vector_t;
  ...
};

// CamelCase (w/ upper case initial) for namespaces
namespace Something {
...
}
// The only exception is the `detail` namespace within another one.
// `detail` must never be the top-most namespace
namespace Foo {
namespace detail {
...
}
}

// mixedCase (w/ lower case initial) for functions and methods
doSomething(...);
thing.doSomething(...);
// same conventions for implementation details
// there are quite a few cases where details helpers use snake_case instead. these functions
// are not fundamentally different from regular functions and are already marked as special
// by virtue of being in the `detail` namespace.
detail::anImplementationDetail(...);

// mixedCase (with appropriate prefixes) for variables
int aVariable;
struct Thing {
  double aPublicVariable;
  float anotherPublicOne;
};
class Foo {
private:
  m_privateMember;
};
// Both for globals (eeww) and static class/struct members
static int s_doesNotChange = -42;

// CamelCase with k prefix for constants
// Constants must always be `constexpr`
static constexpr double kMagic = 1.23;
// Exception: unit definitions in the `Acts::UnitConstants` namespace

// CamelCase with e prefix for enum values
// These are not really constants but symbolic values, e.g. they can never have an address.
enum Bar {
  eBla,
  eBlub,
};
// enums should be `enum class` unless they can not be.

// ALL_UPPER_CASE with ACTS_ prefix for macros
ACTS_NEVER_USE_MACROS(...)

I have another suggestion: every separate part of a class/function declaration/definition should be in a separate line, including each argument, like this:

template <typename propagator_t,
          typename updater_t = VoidKalmanUpdater,
          typename smoother_t = VoidKalmanSmoother,
          typename outlier_finder_t = VoidOutlierFinder,
          typename calibrator_t = VoidMeasurementCalibrator,
          typename input_converter_t = VoidKalmanComponents,
          typename output_converter_t = VoidKalmanComponents>

...

template< typename a,
          typename b>
Result<b>
functionName(
          int aPrettyLongNameDescribingTheVariable,
          a parA,
          b parC,
          Cache parB,
          double anotherPrettyLongVariableName)
          const;         

I think it's much easier to read than having an arbitrary number of arguments per line:

template <typename propagator_t, typename updater_t = VoidKalmanUpdater,
          typename smoother_t = VoidKalmanSmoother,
          typename outlier_finder_t = VoidOutlierFinder,
          typename calibrator_t = VoidMeasurementCalibrator,
          typename input_converter_t = VoidKalmanComponents,
          typename output_converter_t = VoidKalmanComponents>

...
template< typename a, typename b>
Result<b> functionName(int aPrettyLongNameDescribingTheVariable,
                       a parA, b parC, Cache parB, double
                       anotherPrettyLongVariableName) const;

I mostly agree, but that is a formatting issue that should be handled automatically by clang-format. This is not a naming issue. I think, there is a pending MR !626.

I like this very much.

Also, @rlangenb suggestion is good, if we can enforce it buy clang-tidy.

There's one more thing I would like to discuss:

Unfortunately we broadcast from different places what

using SurfaceVector = std::vector<const Surface*>;

or

using SurfacePtrVector = std::vector<std::shared_ptr<const Surface> >;

... you get the point.

Shall we streamline this and promote them e.g. into the Surface.hpp?

Such everywhere in the code SurfacerPtrVector should mean the same ...

One more thing:

Let's also write down some rules about comments, and file headers, i.e. we should remove the (sometimes wrong) file name

/// Filename.h

From the code.

Yes, I will add those.

Regarding the SurfacePtrVector discussion: I think we should not have these typedefs to begin with, e.g. if I see SurfacePtrVector it is not immediately clear if this means

std::vector<Surface*
std::vector<const Surface*>;
std::vector<std::shared_ptr<const Surface>>;
std::vector<std::unique_ptr<Surface>>;
...

This can of course be clarified by using a more presciptive name, e.g.

using SurfaceConstSharedPtrContainer = std::vector<std::shared_ptr<const Surface>>;

at which point the typedef is about as along as the original type. My preference would be to get rid of these typedefs altogether and just use e.g. std::vector<std::shared_ptr<const Surface>> directly in a global context. In a local context, it might still be useful to provide typedefs to simplify our code, but in that case the name should be semantical and not merely descriptive, e.g.

struct SurfaceAlgorithm {
  using InputSurfaces = std::vector<std::shared_ptr<const Surface>>;
  ...
};

Regardless, this is more of a general code style issue and not merely naming.

That's true indeed.

Anyhow, we should never have e.g. Surface* unless someone explicitly casts the constness away for surfaces.

In the new test version of the geometry without layers, I actually managed to go around the constness problem we have in the geometry building ... but that's a discussion for another day.

mentioned in merge request !796 (merged)

created merge request !796 (merged) to address this issue

changed milestone to %0.20.00

added github-migrate label

closed via merge request !796 (merged)

mentioned in commit ffafb39c

mentioned in merge request atlas/atlasexternals!660 (merged)