Good or bad: Calling destructor in constructor [closed]

Question

---

Break: I don't think it is the same question actually, the other question is a general question about calling destructors manually. This is at the creating process, inside the class itself. Still want to know what happen when you do this, like stated in the question below.

---

At first, I think it is bad, real bad. Just analysing this piece of code of a constructor (see below), made by two guys and need it to translate it to Delphi object Pascal. It must behave the same like the C-version. I don't like the style, very ugly but never mind.

Another thing, at two stages in the code it calls the destructor when fail (I suppose to close the connection however the destructor is automaticly called when deleted, why want you do this anyway?). I think that is not the way to do it or do miss something inhere?

Also, after calling the destructor, they want to throw an exception (huh?) however I think this will never be executed and cause another exeption when you manually want to access it or want to delete it.

---

Serial::Serial(
  std::string &commPortName,
  int bitRate,
  bool testOnStartup,
  bool cycleDtrOnStartup
) {
  std::wstring com_name_ws = s2ws(commPortName);

  commHandle =
    CreateFileW(
      com_name_ws.c_str(),
      GENERIC_READ | GENERIC_WRITE,
      0,
      NULL,
      OPEN_EXISTING,
      0,
      NULL
    );

  if(commHandle == INVALID_HANDLE_VALUE)
    throw("ERROR: Could not open com port");
  else {
    // set timeouts
    COMMTIMEOUTS timeouts;

    /* Blocking:
        timeouts.ReadIntervalTimeout = MAXDWORD;
        timeouts.ReadTotalTimeoutConstant = 0;
        timeouts.ReadTotalTimeoutMultiplier = 0;
       Non-blocking:
        timeouts = { MAXDWORD, 0, 0, 0, 0}; */

    // Non-blocking with short timeouts
    timeouts.ReadIntervalTimeout = 1;
    timeouts.ReadTotalTimeoutMultiplier = 1;
    timeouts.ReadTotalTimeoutConstant = 1;
    timeouts.WriteTotalTimeoutMultiplier = 1;
    timeouts.WriteTotalTimeoutConstant = 1;

    DCB dcb;
    if(!SetCommTimeouts(commHandle, &timeouts)) {
      Serial::~Serial();                                      <- Calls destructor!
      throw("ERROR: Could not set com port time-outs");
    }

    // set DCB; disabling harware flow control; setting 1N8 mode
    memset(&dcb, 0, sizeof(dcb));
    dcb.DCBlength = sizeof(dcb);
    dcb.BaudRate = bitRate;
    dcb.fBinary = 1;
    dcb.fDtrControl = DTR_CONTROL_DISABLE;
    dcb.fRtsControl = RTS_CONTROL_DISABLE;
    dcb.Parity = NOPARITY;
    dcb.StopBits = ONESTOPBIT;
    dcb.ByteSize = 8;

    if(!SetCommState(commHandle, &dcb)) {
      Serial::~Serial();                                    <- Calls destructor!
      throw("ERROR: Could not set com port parameters");
    }
  }

  if(cycleDtrOnStartup) {
    if(!EscapeCommFunction(commHandle, CLRDTR))
      throw("ERROR: clearing DTR");
    Sleep(200);
    if(!EscapeCommFunction(commHandle, SETDTR))
      throw("ERROR: setting DTR");
  }

  if(testOnStartup) {
    DWORD numWritten;
    char init[] = "PJON-python init";
    if(!WriteFile(commHandle, init, sizeof(init), &numWritten, NULL))
      throw("writing initial data to port failed");
    if(numWritten != sizeof(init))
      throw("ERROR: not all test data written to port");
  }
};

Serial::~Serial() {
  CloseHandle(commHandle);
};

// and there is more etc .......
// .............

---

Next question, what will actually happen in memory when executing this code and it calls the destructor? I am not able to execute it and debug it.

Answer 1

This code is ugly but legal. When an exception is thrown from constructor, the corresponding destructor is never called. So calling it manually before throwing is needed to prevent the resource leak. The real bug here is not calling destructor manually in other cases before throwing exception.

Of course, a better way of doing this is having a separate RAII object that encapsulates commHandle. A unique_ptr with custom deleter can serve this role.

Any destructor beyond low-level libraries is a code smell in modern C++.

Answer 2

Well, let's start by saying the obvious: don't write code this way. I can see why they did it - calling the destructor manually was a convenient way to clean up before throwing that exception, but why is it such a bad idea?

Well, the destructor is normally only called if the constructor runs to completion (so it won't be run, in the normal way of things, if the constructor throws) and this is deliberate as it allows the destructor to assume that the object has been fully initialised. A destructor of any complexity that tries to tear down an object which is not fully initialised is likely to run into trouble.

Now none of this matters in the code as written, because all we have here is a tinpot destructor that just closes the handle so, here, the code does correctly clean up before throwing (sometimes, thank you Eugene) and we can all sit down and relax. But as a programming pattern it stinks, and, now that you know what it actually does you should tidy it up when you move it to Delphi.

So, lecture over, a few specifics (in no particular order):

• When you call a destructor manually, it's just like calling any other function - it is executed and it returns and life goes on. Specifically, the object itself is not deallocated. Doing this has value when using placement new.
• It follows from the above that that call to throw will be executed after the destructor returns (it would be anyway, regardless).
• Just to repeat, when a constructor throws, the destructor is not called. The object will be deallocated subsequently however, before the exception is caught (if it ever is), I believe.

If the rest of the code you have to convert is written in such a slapdash manner, I don't envy you. Constructors shouldn't fail anyway, in the general run of things, just open the port in a separate method once the object is up and running.