Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Naming

#Naming NamesNames beginning with an underscore followed immediately are reserved to the implementation for any purpose (which means they might be defined as macros). Personally, I always avoid leading underscores in identifiers, as that's easier than learning the detailed rules. Even if you do know the rules, _Registry() is a complete no-no.

Thread safety

#Thread safety NoneNone of the accesses to the std::unordered_set have any locking, so it's up to the calling to to arrange that. Difficult at the best of times, but close to impossible for destructors. I think that InstanceRegistry requires a std::mutex that can be obtained using a std::lock_guard (or perhaps std:scoped_lock in C++17) in each of the registry methods.

Avoid the need to inherit

#Avoid the need to inherit WeWe already have a mechanism for performing actions when an object is deleted, and that is the smart pointer. If you're able to insist that your objects are always heap-allocated, then you may be able to get the same effect for much less work by writing a factory method that creates and registers objects, returning them wrapped in a smart pointer with a deregistering deleter. The deleter simply removes the object's registration before invoking std::default_deleter<T>() (or perhaps your preferred custom deleter).

Another way to be able to register objects whose base classes you don't control is to use the Curiously Recurring Template Pattern (CRTP) to add the functionality in the subclass rather than the base class.

I'm not saying that either of the above are necessarily improvements, but at least be aware that they are options.

#Naming Names beginning with an underscore followed immediately are reserved to the implementation for any purpose (which means they might be defined as macros). Personally, I always avoid leading underscores in identifiers, as that's easier than learning the detailed rules. Even if you do know the rules, _Registry() is a complete no-no.

#Thread safety None of the accesses to the std::unordered_set have any locking, so it's up to the calling to to arrange that. Difficult at the best of times, but close to impossible for destructors. I think that InstanceRegistry requires a std::mutex that can be obtained using a std::lock_guard (or perhaps std:scoped_lock in C++17) in each of the registry methods.

#Avoid the need to inherit We already have a mechanism for performing actions when an object is deleted, and that is the smart pointer. If you're able to insist that your objects are always heap-allocated, then you may be able to get the same effect for much less work by writing a factory method that creates and registers objects, returning them wrapped in a smart pointer with a deregistering deleter. The deleter simply removes the object's registration before invoking std::default_deleter<T>() (or perhaps your preferred custom deleter).

Another way to be able to register objects whose base classes you don't control is to use the Curiously Recurring Template Pattern (CRTP) to add the functionality in the subclass rather than the base class.

I'm not saying that either of the above are necessarily improvements, but at least be aware that they are options.

Naming

Names beginning with an underscore followed immediately are reserved to the implementation for any purpose (which means they might be defined as macros). Personally, I always avoid leading underscores in identifiers, as that's easier than learning the detailed rules. Even if you do know the rules, _Registry() is a complete no-no.

Thread safety

None of the accesses to the std::unordered_set have any locking, so it's up to the calling to to arrange that. Difficult at the best of times, but close to impossible for destructors. I think that InstanceRegistry requires a std::mutex that can be obtained using a std::lock_guard (or perhaps std:scoped_lock in C++17) in each of the registry methods.

Avoid the need to inherit

We already have a mechanism for performing actions when an object is deleted, and that is the smart pointer. If you're able to insist that your objects are always heap-allocated, then you may be able to get the same effect for much less work by writing a factory method that creates and registers objects, returning them wrapped in a smart pointer with a deregistering deleter. The deleter simply removes the object's registration before invoking std::default_deleter<T>() (or perhaps your preferred custom deleter).

Another way to be able to register objects whose base classes you don't control is to use the Curiously Recurring Template Pattern (CRTP) to add the functionality in the subclass rather than the base class.

I'm not saying that either of the above are necessarily improvements, but at least be aware that they are options.

Source Link
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325

#Naming Names beginning with an underscore followed immediately are reserved to the implementation for any purpose (which means they might be defined as macros). Personally, I always avoid leading underscores in identifiers, as that's easier than learning the detailed rules. Even if you do know the rules, _Registry() is a complete no-no.

#Thread safety None of the accesses to the std::unordered_set have any locking, so it's up to the calling to to arrange that. Difficult at the best of times, but close to impossible for destructors. I think that InstanceRegistry requires a std::mutex that can be obtained using a std::lock_guard (or perhaps std:scoped_lock in C++17) in each of the registry methods.

#Avoid the need to inherit We already have a mechanism for performing actions when an object is deleted, and that is the smart pointer. If you're able to insist that your objects are always heap-allocated, then you may be able to get the same effect for much less work by writing a factory method that creates and registers objects, returning them wrapped in a smart pointer with a deregistering deleter. The deleter simply removes the object's registration before invoking std::default_deleter<T>() (or perhaps your preferred custom deleter).

Another way to be able to register objects whose base classes you don't control is to use the Curiously Recurring Template Pattern (CRTP) to add the functionality in the subclass rather than the base class.

I'm not saying that either of the above are necessarily improvements, but at least be aware that they are options.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /