Update coding-style
Description
Our coding style has evolved a bit over years, we need to update it.
Proposal
- Signal/slots conventions #54 (closed)
- Case (snake case enabled in some cases)
- Types naming
Functional specifications
Switch to snake_case
Yeah, this is a big change, but that is the most consensual choice in the end. Besides personal preferences, here are the main reasons that led this choice:
- this is the coding style of the STL. Following the latest C++ trends is part of the DNA of Sight, this is just a step further.
- the other language used by the team is Python. This will make collaboration easier.
Don't worry, I will take care of the code base conversion mostly in my free time... I have already started experiments, clang-tidy might help and will help for sure in the future to enforce the new coding style.
Implications
IAbstractClass
is disgusting in snake case: i_abstract_class
. I found some arguments that convince me that this is not the best idea. We simply remove the letter prefix in favour of a cleverer naming. Example:
Instead of:
classDiagram
`io::base::service::IGrabber` <|-- `module::io::pcl::SFrameGrabber`
`io::base::service::IGrabber` <|-- `module::io::video::SFrameGrabber`
`io::base::service::IGrabber` <|-- `io::base::service::IRGBDGrabber`
`io::base::service::IRGBDGrabber` <|-- `module::io::realsense::SScan`
do something like:
classDiagram
`io::base::service::grabber` <|-- `module::io::pcl::frame_grabber`
`io::base::service::grabber` <|-- `module::io::video::frame_grabber`
`io::base::service::grabber` <|-- `io::base::service::rgbd_grabber`
`io::base::service::rgbd_grabber` <|-- `module::io::realsense::grabber`
The namespace brings the evidence of who is an interface (mention of base) and who is not.
Other example:
classDiagram
`service::IService` <|-- `ui::base::IEditor`
`ui::base::IEditor` <|-- `module::ui::qt::SParameters`
`service::base` <|-- `ui::service::editor`
`ui::service::editor` <|-- `module::ui::qt::parameters`
Which avoids repetitions. The only downside is that we can no longer have the same term for a class and a namespace. So we can not have for instance a class sight::service
and a class sight::service::controller
. That's why I propose to use base
as a class name when the interface lies in the same namespace, so here sight::service::base
. In this example, the term ui::service::editor
clearly documents that module::ui::qt::parameters
is a UI service.
When we have a base
root class, I would also propose to move public types previously defined in IService directly in the sight::service::namespace
in order to shorten the syntax. So for instance sight::service::IService::KeyConnectionsMap
would translate into sight::service::key_connections_map_t
, or maybe even sight::service::connections_t
.
In this way, I would argue to always have the term service
present in the namespace of a service base class. This would imply to rename or move several classes to harmonize this, probably later:
sight::io::base::service::* -> sight::io::service::*
sight::io::base::service::* -> sight::io::service::*
sight::ui::base::IAction -> sight::ui::service::action
sight::ui::base::IEditor -> sight::ui::service::editor
For the adaptors, I would let them as is apart from the renaming, since they are a special case
sight::viz::scene3d::IAdaptor -> sight::viz::scene3d::adaptor
Also, New
translates to new
. Which of course is prohibited. I propose to rename it to a synonym, i.e. create()
. However, we could also argue that in most cases, this New
is just an alias to std::make_shared<>
. I don't think it is a good idea to force to use this pointer type everywhere. I would only keep the new create
when necessary for object creation through factories or interface, and use raw C++ otherwise. Thus:
auto w = core::thread::Worker::New();
// Becomes create() because Worker is abstract
auto w = core::thread::worker::create();
auto image = sight::data::Image::New();
// Becomes make_shared() because we are instantiating the concrete type
auto image = std::make_shared<sight::data::Image>();
I would take the opportunity to remove most factory declarations (3rd parameter of SIGHT_DECLARE_SERVICE) that are useless most of the time these days. I would also remove the (in)famous Key
factory used in many constructors, which was initially just there to prevent default constructors from being called. Why, I don't have a fucking clue... Maybe, again, enforce a specific allocation of objects.
Last, dynamicCast
translates to dynamic_cast
. Again. I would simply argue to use std::dynamic_pointer_cast<T>
instead. No need for aliases that bring confusion. dynamicConstCast
for instance is not a const_cast
, but an alias for std::dynamic_pointer_cast<const T>
. So I strongly suggest sticking with the STL casts.
Types naming
We officialize the use of _t
suffix instead of Type
.
API privacy
The detail
namespace should be advertised in the guide to separate the private API. For private implementation outside of a detail
namespace, the Impl
suffix must be added to the implementation class.
Technical specifications
Details of the implementation
Test plan
Describe how you will verify that the implementation fulfils the specifications