epics-base / pvDataCPP

pvDataCPP is an EPICS V4 C++ module
https://epics-base.github.io/pvDataCPP/
Other
6 stars 16 forks source link

ByteBuffer cleanup #16

Closed mdavidsaver closed 8 years ago

mdavidsaver commented 8 years ago

Check for allocation failure in ctors and add const qualifier where possible. Also document that externally provided buffers will be passed to free() (so can't use new[]).

mdavidsaver commented 8 years ago

oops, push the wrong branch

dhickin commented 8 years ago
  1. When constructing a ByteBuffer around an existing buffer it's not true that the buffer is "free'd with free()". The wrapping constructor sets _wrapped to true and free is then not called in the destructor. Presumably whoever created the buffer is responsible for deleting it using the correct method.
  2. Is std::bad_alloc the right exception to throw in the wrapping constructor? No allocation has been attempted. If it's not valid to call this constructor with a null pointer it seems reasonable to throw an exception, but std::invalid_argument might be a more correct choice.
mdavidsaver commented 8 years ago
  1. This was a stupid mistake on my part. reverted.
  2. Constructing invalid_argument involves memory allocation, bad_alloc doesn't. So if an actual low memory situation exists, bad_alloc will be thrown. So the caller must be prepared for bad_alloc anyway (as most all c++ code should). That said, I don't feel that strongly about this. If it's a blocker in your mind I'll change it.
dhickin commented 8 years ago

My preference is for std::invalid_argument. It's logically more correct, as no allocation might have been performed. If the buffer is allocated with new[] or some similar mechanism it will (usually, unless exceptions are disabled) throw the bad_alloc.If you allocate with malloc, I would say that the client code should check the pointer and throw the bad_alloc. The case when a null pointer is used (e.g. if it's initialised to null and never assigned to with the result of a allocation) will then return invalid_argument .

mdavidsaver commented 8 years ago

Changed to invalid_argument. Unless there are further objections I'll merge this on Monday.