Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Barthelemy
Copy link
Collaborator

No description provided.

@Barthelemy
Copy link
Collaborator Author

(I hope boost frees the memory returned by what())

Copy link
Contributor

@matthiasrichter matthiasrichter left a 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);
Copy link
Contributor

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?

include/Common/Exceptions.h Show resolved Hide resolved
@Barthelemy
Copy link
Collaborator Author

Passing a string to the exception with << is not possible at the moment, it will trigger a compilation error.

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 diagnostic_information(), then raise again if it is fatal. This way we are sure to print all the extra information added.

@MichaelLettrich
Copy link

MichaelLettrich commented May 14, 2020

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)

@Barthelemy
Copy link
Collaborator Author

I am trying to build on my mac but it decided to rebuild Clang 😠

@MichaelLettrich
Copy link

@Barthelemy: @ktf told me that a new tag for O2 should fix it. Let's see if it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants