Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Naming

Naming

I would rename initialize because it's more of a setter. You can usually initialize an object only once (typically in the constructor), but there is no problem with calling initialize twice

#Accessing the object

Accessing the object

There is no method to access the object. Maybe that's wanted because you are using friendship of some kind that you didn't paste? If that is the case, I highly recommend that you use the Attorney Client idiom if you are not already, so that your friend class/method can only access the m_Data member and not the boolean

#Additional features

Additional features

There are some features you may (or may not) want to add:

  • .get() method
  • T&& constructor
  • bool conversion operator
  • indirection operator
  • structure dereference operator

#Naming

I would rename initialize because it's more of a setter. You can usually initialize an object only once (typically in the constructor), but there is no problem with calling initialize twice

#Accessing the object

There is no method to access the object. Maybe that's wanted because you are using friendship of some kind that you didn't paste? If that is the case, I highly recommend that you use the Attorney Client idiom if you are not already, so that your friend class/method can only access the m_Data member and not the boolean

#Additional features

There are some features you may (or may not) want to add:

  • .get() method
  • T&& constructor
  • bool conversion operator
  • indirection operator
  • structure dereference operator

Naming

I would rename initialize because it's more of a setter. You can usually initialize an object only once (typically in the constructor), but there is no problem with calling initialize twice

Accessing the object

There is no method to access the object. Maybe that's wanted because you are using friendship of some kind that you didn't paste? If that is the case, I highly recommend that you use the Attorney Client idiom if you are not already, so that your friend class/method can only access the m_Data member and not the boolean

Additional features

There are some features you may (or may not) want to add:

  • .get() method
  • T&& constructor
  • bool conversion operator
  • indirection operator
  • structure dereference operator
deleted 152 characters in body
Source Link
Maliafo
  • 278
  • 1
  • 9

#Data

I really don't see the purpose of this structure, as it is only a wrapper for the data that basically does nothing. Why not use T directly?

#Destructor

In your destructor, you explicitly call m_Data's destructor

 m_Data.m_Data.~T();

That is a very bad thing that will result in an Undefined Behavior. The link concerns local variable, but this apply to member variables too: when your object is destroyed, all its member have their destructors automatically invoked.

You have the same problem in the assignment operator. As a rule of thumb, remember that unless using placement new , you should never explicitly call a destructor.

#Naming

I would rename initialize because it's more of a setter. You can usually initialize an object only once (typically in the constructor), but there is no problem with calling initialize twice

#Accessing the object

There is no method to access the object. Maybe that's wanted because you are using friendship of some kind that you didn't paste? If that is the case, I highly recommend that you use the Attorney Client idiom if you are not already, so that your friend class/method can only access the m_Data member and not the boolean

#Additional features

There are some features you may (or may not) want to add:

  • .get() method
  • T&& constructor
  • bool conversion operator
  • indirection operator
  • structure dereference operator

#Data

I really don't see the purpose of this structure, as it is only a wrapper for the data that basically does nothing. Why not use T directly?

#Destructor

In your destructor, you explicitly call m_Data's destructor

 m_Data.m_Data.~T();

That is a very bad thing that will result in an Undefined Behavior. The link concerns local variable, but this apply to member variables too: when your object is destroyed, all its member have their destructors automatically invoked.

You have the same problem in the assignment operator. As a rule of thumb, remember that unless using placement new , you should never explicitly call a destructor.

#Naming

I would rename initialize because it's more of a setter. You can usually initialize an object only once (typically in the constructor), but there is no problem with calling initialize twice

#Accessing the object

There is no method to access the object. Maybe that's wanted because you are using friendship of some kind that you didn't paste? If that is the case, I highly recommend that you use the Attorney Client idiom if you are not already, so that your friend class/method can only access the m_Data member and not the boolean

#Additional features

There are some features you may (or may not) want to add:

  • .get() method
  • T&& constructor
  • bool conversion operator
  • indirection operator
  • structure dereference operator

#Naming

I would rename initialize because it's more of a setter. You can usually initialize an object only once (typically in the constructor), but there is no problem with calling initialize twice

#Accessing the object

There is no method to access the object. Maybe that's wanted because you are using friendship of some kind that you didn't paste? If that is the case, I highly recommend that you use the Attorney Client idiom if you are not already, so that your friend class/method can only access the m_Data member and not the boolean

#Additional features

There are some features you may (or may not) want to add:

  • .get() method
  • T&& constructor
  • bool conversion operator
  • indirection operator
  • structure dereference operator
Source Link
Maliafo
  • 278
  • 1
  • 9

#Data

I really don't see the purpose of this structure, as it is only a wrapper for the data that basically does nothing. Why not use T directly?

#Destructor

In your destructor, you explicitly call m_Data's destructor

 m_Data.m_Data.~T();

That is a very bad thing that will result in an Undefined Behavior. The link concerns local variable, but this apply to member variables too: when your object is destroyed, all its member have their destructors automatically invoked.

You have the same problem in the assignment operator. As a rule of thumb, remember that unless using placement new, you should never explicitly call a destructor.

#Naming

I would rename initialize because it's more of a setter. You can usually initialize an object only once (typically in the constructor), but there is no problem with calling initialize twice

#Accessing the object

There is no method to access the object. Maybe that's wanted because you are using friendship of some kind that you didn't paste? If that is the case, I highly recommend that you use the Attorney Client idiom if you are not already, so that your friend class/method can only access the m_Data member and not the boolean

#Additional features

There are some features you may (or may not) want to add:

  • .get() method
  • T&& constructor
  • bool conversion operator
  • indirection operator
  • structure dereference operator
lang-cpp

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