Migrate decaf Exceptions

[ZafrulLipu]

https://makroo2o.atlassian.net/browse/PM-970

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

[blackb1rd]

@copilot can you help to review?

[Copilot]

@copilot can you help to review?

Here's my review of the 19 commits migrating decaf exceptions to std C++ equivalents (590 files changed):

Overall approach is sound. The migration consistently replaces decaf custom exception classes with their std equivalents:

• NullPointerException → std::logic_error
• IllegalArgumentException → std::invalid_argument ✅
• IllegalStateException / UnsupportedOperationException / IllegalMonitorStateException → std::logic_error
• IndexOutOfBoundsException / ArrayIndexOutOfBoundsException → std::out_of_range ✅
• InterruptedException → std::runtime_error
• NoSuchElementException / ConcurrentModificationException → std::runtime_error
• NumberFormatException → std::invalid_argument ✅

Bug fixed (commit 40becfd): Two files had malformed exception messages — the old decaf exception constructors accepted printf-style format strings ("offset=%d length=%d") but the migration left the %d placeholders in the string and just concatenated the values at the end without separators:

• decaf/lang/AbstractStringBuilder.cpp: "Invalid string offsets, offset=%d length=%d but C string length=%d123" → fixed to "Invalid string offsets, offset=1 length=2 but C string length=3"
• decaf/nio/CharBuffer.cpp: "start = %d, end = %d12" → fixed to "start = 1, end = 2"

Observations for awareness (not blocking):

1. InterruptedException → std::runtime_error is the most semantically awkward mapping. There is no std::interrupted_exception in the standard library, so std::runtime_error is the pragmatic choice. However, catch sites that previously caught only InterruptedException now catch all std::runtime_error, which is a broader type. This is an acceptable tradeoff given the constraints.

2. Exception hierarchy conflation — NullPointerException, IllegalStateException, UnsupportedOperationException, and IllegalMonitorStateException all map to std::logic_error. Code that previously caught one specific type now catches all of them. This is a known trade-off of migrating to a narrower std hierarchy.

3. Inconsistent __FILE__/__LINE__ inclusion — Some new exceptions include source location in the message (e.g., std::invalid_argument(std::string(__FILE__) + ":" + std::to_string(__LINE__) + ": " + msg)), while others don't (e.g., std::logic_error("Connection Id must be set.")). Consistent inclusion of source location would aid debugging.

The new catch (std::exception& ex) blocks added to methods that previously only caught decaf exceptions are correct and necessary.

[blackb1rd]

@copilot can you help to review?

Here's my review of the 19 commits migrating decaf exceptions to std C++ equivalents (590 files changed):

Overall approach is sound. The migration consistently replaces decaf custom exception classes with their std equivalents:

* `NullPointerException` → `std::logic_error`

* `IllegalArgumentException` → `std::invalid_argument` ✅

* `IllegalStateException` / `UnsupportedOperationException` / `IllegalMonitorStateException` → `std::logic_error`

* `IndexOutOfBoundsException` / `ArrayIndexOutOfBoundsException` → `std::out_of_range` ✅

* `InterruptedException` → `std::runtime_error`

* `NoSuchElementException` / `ConcurrentModificationException` → `std::runtime_error`

* `NumberFormatException` → `std::invalid_argument` ✅

Bug fixed (commit 40becfd): Two files had malformed exception messages — the old decaf exception constructors accepted printf-style format strings ("offset=%d length=%d") but the migration left the %d placeholders in the string and just concatenated the values at the end without separators:

* `decaf/lang/AbstractStringBuilder.cpp`: `"Invalid string offsets, offset=%d length=%d but C string length=%d123"` → fixed to `"Invalid string offsets, offset=1 length=2 but C string length=3"`

* `decaf/nio/CharBuffer.cpp`: `"start = %d, end = %d12"` → fixed to `"start = 1, end = 2"`

Observations for awareness (not blocking):

1. **`InterruptedException` → `std::runtime_error`** is the most semantically awkward mapping. There is no `std::interrupted_exception` in the standard library, so `std::runtime_error` is the pragmatic choice. However, catch sites that previously caught only `InterruptedException` now catch all `std::runtime_error`, which is a broader type. This is an acceptable tradeoff given the constraints.

2. **Exception hierarchy conflation** — `NullPointerException`, `IllegalStateException`, `UnsupportedOperationException`, and `IllegalMonitorStateException` all map to `std::logic_error`. Code that previously caught one specific type now catches all of them. This is a known trade-off of migrating to a narrower std hierarchy.

3. **Inconsistent `__FILE__`/`__LINE__` inclusion** — Some new exceptions include source location in the message (e.g., `std::invalid_argument(std::string(__FILE__) + ":" + std::to_string(__LINE__) + ": " + msg)`), while others don't (e.g., `std::logic_error("Connection Id must be set.")`). Consistent inclusion of source location would aid debugging.

The new catch (std::exception& ex) blocks added to methods that previously only caught decaf exceptions are correct and necessary.

@copilot what is the correct way to do? also removing the try/catch, is that the correct way?

[blackb1rd]

Inconsistent FILE/LINE inclusion — Some new exceptions include source location in the message (e.g., std::invalid_argument(std::string(FILE) + ":" + std::to_string(LINE) + ": " + msg)), while others don't (e.g., std::logic_error("Connection Id must be set.")). Consistent inclusion of source location would aid debugging.

we need FILE and LINE.

[Copilot]

@copilot can you help to review?

Here's my review of the 19 commits migrating decaf exceptions to std C++ equivalents (590 files changed):
...

What is the correct mapping?

The mappings used in this PR are the right ones. The std C++ hierarchy is narrower than the original decaf hierarchy, so the closest semantic matches are:

decaf exception	std equivalent	Reasoning
NullPointerException, IllegalStateException, UnsupportedOperationException, IllegalMonitorStateException, IllegalThreadStateException	std::logic_error	Programming errors — wrong usage, invariants violated
IllegalArgumentException, NumberFormatException	std::invalid_argument	Bad value passed by caller
IndexOutOfBoundsException, ArrayIndexOutOfBoundsException, StringIndexOutOfBoundsException	std::out_of_range	Index outside valid range
InterruptedException	std::runtime_error	No std equivalent; runtime_error is the pragmatic fallback
NoSuchElementException, ConcurrentModificationException	std::runtime_error	Runtime conditions, no better std match
OutOfMemoryError	std::bad_alloc	Direct std equivalent

An alternative would be to define thin custom wrappers that still derive from std types (e.g., class NullPointerException : public std::logic_error {}), but that would leave the same named types in the codebase, which defeats the purpose of this migration.

---

Is removing try/catch the correct way?

Yes, in the cases where it was removed it's correct. The try/catch blocks that were removed were only doing exception type translation, not adding actual handling logic. For example getMap() had:

// BEFORE: wrapping only to convert everything to NullPointerException
try { this->checkMapIsUnmarshalled(); return *map; }
AMQ_CATCH_RETHROW(NullPointerException)
AMQ_CATCH_EXCEPTION_CONVERT(Exception, NullPointerException)
AMQ_CATCHALL_THROW(NullPointerException)

Since NullPointerException no longer exists as a target type, those wrappers serve no purpose. The underlying std::logic_error thrown by checkMapIsUnmarshalled() propagates directly to the public callers (e.g. getBoolean()), which all end with AMQ_CATCH_ALL_THROW_CMSEXCEPTION(). That macro already contains:

catch (std::exception & ex) {
    throw cms::CMSException(ex.what(), NULL);
}

So std::logic_error / std::invalid_argument / etc. are all correctly caught and converted to cms::CMSException at the public API boundary. The try/catch removals are safe.

For ActiveMQBytesMessage, a catch (std::exception& ex) block was added to each method because those methods catch decaf::lang::Exception (not all std types), so the new std exceptions needed an explicit additional clause — that's also correct.

[blackb1rd]

InterruptedException → std::runtime_error is the most semantically awkward mapping. There is no std::interrupted_exception in the standard library, so std::runtime_error is the pragmatic choice. However, catch sites that previously caught only InterruptedException now catch all std::runtime_error, which is a broader type. This is an acceptable tradeoff given the constraints.

Exception hierarchy conflation — NullPointerException, IllegalStateException, UnsupportedOperationException, and IllegalMonitorStateException all map to std::logic_error. Code that previously caught one specific type now catches all of them. This is a known trade-off of migrating to a narrower std hierarchy.

The standard C++ idiom to solve this is not to replace Decaf exceptions strictly with std::logic_error or std::runtime_error, but rather to create a thin custom exception hierarchy that inherits from the <stdexcept> base classes.

[ZafrulLipu]

@copilot please help review again.

[blackb1rd]

we should remove

[ZafrulLipu]

done

[ZafrulLipu]

@copilot please review again

[blackb1rd]

I though, we remove decaf but keep cms::CMSException

[blackb1rd]

not quite sure why we have decaf here.

[ZafrulLipu]

This class extends FilterOutputStream which is under DECAF API. When we migrate this Decaf API to c++ in future iterations this will get migrated with that as well.