GrammaTech / gtirb

Intermediate Representation for Binary analysis and transformation
https://grammatech.github.io/gtirb/
Other
305 stars 36 forks source link

No longer throw exceptions from our APIs. #15

Closed AaronBallman closed 5 years ago

AaronBallman commented 5 years ago

This removes the explicit throws from our APIs. On error,

Open questions:

  1. Should we remove all of the EXPECT_NO_THROW (et al) calls in the testing code?
  2. Should we disable exceptions and RTTI in CMake?
  3. Should we fix the setData() calls that may leave the container in an inconsistent state? (This is not a new bug with this branch.)

My thoughts are:

  1. Yes, I think these can just go away.
  2. I don't have the CMake chops to do this in such a way that will automatically inherit the decisions made by a project using gtirb directly (such as in a static library build). So I worry that we disable RTTI and exceptions, but someone using gtirb enables exceptions and tries to throw through a gtirb API. I think that leaving the default settings is a safe option for the moment, but can be convinced otherwise.
  3. I think we should fix ImageByteMap::setData(). We have two overloads (setData(Addr Ea, size_t Bytes, std::byte Value) and setData(Addr Ea, const std::array<T, Size>& Data) that will mutate part of the contained data and still return false, leaving the container in an unknown state.
tjohnson-gt commented 5 years ago

My gut feeling:

  1. Yes, get rid of them.
  2. I think we should not disable RTTI or exceptions in CMake by default. Clients can do so if they want to.
  3. Yes, we should fix the overloads to be transactional.
AaronBallman commented 5 years ago
  1. Is done now. One change here is that ImageByteMap::getData() now returns a std::optional<T> instead of a T so that callers can tell if they retrieved valid data.
  2. Is also done (because I don't have to do anything).
  3. Working on this now, but it will require adding a method to ByteMap to see whether the given address and size would cause a region overlap. Then ImageByteData::setData() can call that before setting individual elements in a loop.
AaronBallman commented 5 years ago
  1. is now taken care of as well.