Skip to content

Conversation

dalance
Copy link
Contributor

@dalance dalance commented Jan 14, 2020

If drop has panic, we can't handle error because panic in drop becomes abort.

@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #99 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   66.23%   66.24%   +<.01%     
==========================================
  Files          53       53              
  Lines        5127     5134       +7     
==========================================
+ Hits         3396     3401       +5     
- Misses       1731     1733       +2
Impacted Files Coverage Δ
src/drawing/backend_impl/svg.rs 73.47% <100%> (ø) ⬆️
src/drawing/backend_impl/bitmap.rs 83.06% <100%> (ø) ⬆️
src/coord/datetime.rs 80.47% <0%> (-0.23%) ⬇️
src/drawing/area.rs 94.56% <0%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba598d6...999011b. Read the comment docs.

@38
Copy link
Member

38 commented Jan 15, 2020

Hi @dalance , thanks for the PR. The only thing in my mind is I think we should do something on failure, instead of ignoring it. Since it's weird if the program exit normally, but there's no output.
So I feel it's better to have an error message in this case.

@dalance
Copy link
Contributor Author

dalance commented Jan 16, 2020

I want to handle the error like below:

// chart is plotters::chart::ChartContext
chart
    .plotting_area()
    .present()
    .context(format!("failed to write SVG: {:?}", path))?;

By this code, I expect the following error message.

failed to write SVG: "/a.svg"
  Caused by: backend error: Drawing backend error: Permission denied (os error 13)

But the actual message becomes below:

thread 'main' panicked at 'Unable to save the SVG image: DrawingError(Os { code: 13, kind: PermissionDenied, message: "Permission denied" })', src/libcore/result.rs:1165:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

I think this is not user-friendly, but I can't customize the message because panic in drop.

@38
Copy link
Member

38 commented Jan 16, 2020

Panic in drop is definitely not a good behavior. I just feel uncomfortable with ignoring error either.

But yes, you are right printing an error message might not be friendly to everyone and probably mess up the output. Just looked how standard library handles error during dropping RAII, it seems they also ignores any error.

I probably OK with ignoring it now. Thanks for the explain.

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

Successfully merging this pull request may close these issues.

2 participants