-
Notifications
You must be signed in to change notification settings - Fork 10
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
Better string representation of the ObjectNotFoundException (O2-1416) #36
base: master
Are you sure you want to change the base?
Conversation
(I hope boost frees the memory returned by |
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.
Thanks for looking into this.
I would not restrict this to errinfo_object_name
as information type. Or if it is restricted to that type, all other types should be caught by compile time error. E.g. if one simply uses
ObjectNotFoundError() << "object1"
the current implementation will pretend that the object name is not available, thus ignoring intention of the source code. Though I have not been working with boot::get_error_info
, maybe this is handled transparently.
@@ -29,7 +29,10 @@ struct ObjectNotFoundError : virtual ExceptionBase | |||
{ | |||
const char *what() const noexcept override | |||
{ | |||
return "Object not found error"; | |||
std::string message = "Object not found: "; | |||
auto* errinfo = boost::get_error_info<errinfo_object_name>(*this); |
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 implies that one can only write errinfo_object_name
as additional exception info, correct. If this fails, maybe try to extract string?
Passing a string to the exception with I think that using a errinfo is the correct way of doing it with boost::exception. Of course the extra information has to be provided, under the correct name. I have checked and we do it wherever we use this type of exception in the QC framework. With the proposed fix, the name of the missing object will be printed in the use case you had in the original PR. What will definitely help is catching the exception before it bubbles up outside the framework, print the exception with |
Hi, it seems that there is a problem with building O2 on macOS and arrow/libgandiva... can anyone with a mac check with the lastest alidist if this is a problem with the CI configuration or if this is a genuine problem? @Barthelemy , @ktf CMake Error at dependencies/Findarrow.cmake:24 (set_target_properties):
set_target_properties Can not find target to add properties to:
gandiva_shared
Call Stack (most recent call first):
dependencies/O2Dependencies.cmake:39 (find_package)
dependencies/CMakeLists.txt:11 (include)
CMakeLists.txt:45 (include)
CMake Error at dependencies/Findarrow.cmake:25 (add_library):
add_library cannot create ALIAS target "arrow::gandiva_shared" because
target "gandiva_shared" does not already exist.
Call Stack (most recent call first):
dependencies/O2Dependencies.cmake:39 (find_package)
dependencies/CMakeLists.txt:11 (include)
CMakeLists.txt:45 (include) |
I am trying to build on my mac but it decided to rebuild Clang 😠 |
@Barthelemy: @ktf told me that a new tag for O2 should fix it. Let's see if it does. |
No description provided.