Proper use of memory with dynamic array lengths in c++

Question

When creating dynamic arrays would the following code be considered "correct" in terms of memory use, and performance? Please explain why / why not.

My function getFifoData takes a pointer to a receive buffer, and internally calculates how long the message is based on the current FIFO size using getFifoThreshold.

int serial_spi_handler::getFifoData(unsigned char * rxBuf) {
  uint16_t currentFifoThreshold = getFifoThreshold();
  const int msgLength = (currentFifoThreshold * 2) + 1;

  std::vector < uint8_t > txBuf;
  txBuf.reserve(msgLength);

  uint8_t tBuff[txBuf.size()];
  tBuff[0] = 0xC2;

  int bytesWritten = readWrite(busDescriptor, tBuff, rxBuf, msgLength);

  if (consoleLogging) {
    printf("getFifoData function, wrote: %d bytes\n\r", bytesWritten);
  } else if (diagOutput) {
    qDebug() << "getFifoData function, wrote: " << bytesWritten << " bytes";
  }
  return msgLength;
}

//Header of readWrite:

//int readWrite(int busDescriptor, uint8_t *pTxBuffer, uint8_t *pRxBuffer, int length);

This is definitely not correct, for at least two reasons. Variable lengths arrays are not standard C++. Furthermore, in the shown code, `txBuf.size()` will always be 0, no matter what, and this is certainly not correct. — Sam Varshavchik, Dec 07 '18 at 17:23
@SamVarshavchik I thought VLA’s were part of the C99 standard, it’s just that a surprisingly high number of compilers haven’t gotten around to fully supporting a 20 year old standard. — vandench, Dec 07 '18 at 17:25
Sure. But this is C++, not C. Actually, there's a third reason here: after `txBuf` is used for the sole purpose of determining that its `size()` is 0, it is not used at all from that point on. Something is definitely lost in translation, around here. — Sam Varshavchik, Dec 07 '18 at 17:26
@van And in any case, they are optional in C99 and will hopefully eventually be completely removed. — , Dec 07 '18 at 17:27
@vandench -- VLA's are, indeed, part of C and, presumably, that mythical language C/C++, but they're not part of standard C++. — Pete Becker, Dec 07 '18 at 17:28
excellent point @SamVarshavchik ! By changing txBuf.size() to txBuf.capacity() I should always get a non-zero value. — Eric, Dec 07 '18 at 17:39
That would address only one of three incorrect things here, and does nothing to address the other two. — Sam Varshavchik, Dec 07 '18 at 17:42
@Eric `uint8_t tBuff[txBuf.size()];` -- Why didn't you use `std::vector` here? If it is because you will need a `uint8_t*` later on in the code, then that is what `vector::data()` is for. — PaulMcKenzie, Dec 07 '18 at 17:49
@PaulMcKenzie I didn't realize it could be used like that (but it makes sense), and I wont have 2 instances of it in memory! Does this vector go out of scope at the end of the function like a normal variable or do I have to explicitly delete it? I appreciate all the comments! Ive changed the code to: ```std::vector txBuf; txBuf.reserve(msgLength); txBuf.insert(txBuf.begin(), 0xC2); int bytesWritten = readWrite(busDescriptor, txBuf.data(), rxBuf, msgLength);``` — Eric, Dec 07 '18 at 18:14
A `vector` cleans up the memory it allocates for itself when it goes out of scope. So there is no need to delete it. — PaulMcKenzie, Dec 07 '18 at 18:20

score 0 · Accepted Answer · answered Dec 08 '18 at 21:37

I'm not sure what you mean by "correct"; your code is not correct at least in the sense mentione by @SamVarshavchik, and which a compiler will tell you about:

a.cpp: In function ‘int getFifoData(unsigned char*)’:
a.cpp:20:29: warning: ISO C++ forbids variable length array ‘tBuff’ [-Wvla]
   uint8_t tBuff[txBuf.size()];

If you want to understand why C++ has no VLA's read this SO question:

Why aren't variable-length arrays part of the C++ standard?

Issues with the code

Here are some issues I believe you should consider.

Confusing names

The function readWrite() - does it read? does it write? does it do both? Who knows.
Don't clip names. If you want to name a buffer, call it my_buffer, not my_buff. Similarly, diagnostic_output not diagOutput.
No impromptu initials. What's a tBuffer? Is it a test buffer? A transaction buffer? A transmission buffer? A temporary buffer?
Not everyone knows what RX means.
getFifoData() - what does it even do? Is its parameter a "FIFO"? That's not what the parameter name says. And if it is - where is that information we supposedly get? There's no destination buffer that's passed for use, nor a container that's returned.

Chance of buffer overflow / invalid memory access

Why does getFifoData() take a buffer without also taking its length as well?
Better yet, why can't it take a span?

Use of dynamically-allocated buffers

Both std::vector and the variable-length array are dynamically-allocated memory. VLAs have their own issues (see link above), but as for vectors - you would be performing a memory allocation call on each call of this function; and that might be expensive if it gets called a lot.

Logging

Printing to the console or to a file is slow. Well, sort of slow, anyway - this is all relative. Now, this happens within an "if" statement, but if you've configured your app to log things, you'll be paying this price on every call to getFifoData().

Time it!

Finally, - if you're worried about performance, time your function, or do it with a profiler. Then you can see how much time it actually takes and whether that's a problem.