-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix dac_object_t::verify_type() implementation and uses #1122
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a welcome fix, but also note that there are several other uses of report_invalid_object_id()
that I consider semantically invalid and confusing to users. (See https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1280.) Specifically, all 4 occurrences of this pattern in production/inc/gaia/direct_access/dac_object.inc
are incorrect:
if (!dac_base_t::exists())
{
report_invalid_object_id(gaia::common::c_invalid_gaia_id);
}
The invalid_object_id
exception thrown with c_invalid_gaia_id
implies that the user supplied 0
as the ID (which is what they will see in the error message, not the c_invalid_gaia_id
symbol). But this will be thrown when they dereference any gaia_ptr
that may have been constructed with an ID that was valid at the time but has since been deleted (so the locator wrapped by the gaia_ptr
contains c_invalid_gaia_offset
in the current snapshot, triggering the exception when the locator is dereferenced).
We can certainly address this issue in a separate PR, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I wonder if it makes sense at all to let the user create an object with a known invalid ID. I'd rather throw an exception. If the user was not expecting an invalid ID the exception will stop execution: good. If the user was expecting an invalid ID he/she will have a try-catch.
@@ -24,13 +24,19 @@ namespace direct_access | |||
// | |||
// Exception class implementations. | |||
// | |||
invalid_object_state_internal::invalid_object_state_internal() | |||
{ | |||
m_message = "An operation was attempted on an object that does not exist."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vDonGlover : I am introducing this message to replace the "Cannot find an object with ID '0'." message, which is misleading, because this could also be thrown for objects that were initialized with valid IDs, but then were deleted.
81ac4c7
to
72b42b6
Compare
I could no longer find a way to trigger an assert for an invalid id value.
Instead, I found this issue where
get()
anddelete_row()
methods would report a problem with ID '0' when called with an invalid id value. This was due to theverify_type()
method, which would return id '0' as a way of indicating that the id could not be found. I changed the signature of the method and its uses to eliminate this issue.