darktable-org / darktable-org/rawspeed

Improve error handling throughout the library

Open
#69 16 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
456
Forks
147
PR merge metrics
No merged PRs in 30d

Description

@shoffmeister brought up the question of whether the habit of quoting class/function names in error strings adds value (https://github.com/darktable-org/rawspeed/pull/61#discussion_r99428209). There is a (inconsistently used) pattern that looks like this:

  if (frame.cps != soscps)
    ThrowRDE("LJpegDecompressor::parseSOS: Component number mismatch.");

This pattern has two issues:

  • manually typed class/function name
  • there are different ThrowXXX options available that have questionable value, expecially as they lead to other code patterns like the following:
  try {
    return checkSupportInternal(meta);
  } catch (TiffParserException &e) {
    ThrowRDE("%s", e.what());
  } catch (FileIOException &e) {
    ThrowRDE("%s", e.what());
  } catch (IOException &e) {
    ThrowRDE("%s", e.what());
  }

And that is both verbose and error prone. Both the set of exception classes as well as the way they are used could be improved.

First question: what exception types are useful?

From a library users point of view, I see only two 'types' of problems:

  1. unsupported file: he can't do anything about it, except maybe contact the developers and provide a sample
  2. corrupted file (probably truncated, maybe otherwise 'randomly' broken): he can try to copy it again from his camera, there is no point in contacting the developers.

Unfortunately, the two cases can not reliably be distinguished as long as the library does not implement each and every spec completely (e.g. only one LJpeg predictor mode is currently implemented, so we bail out if predictorMode != 1. We can not say for sure whether that is a valid file with another predictor or a broken file with the right predictor). Even with only 95% accuracy, I'd say it would still be valuable to make that distinction.

From a developers point of view, we might need more diverse types internally, that would have to be determined by inspecting the different catch() sites and see what they actually make of the type. I'd expect to find quite a few inconsistent uses / bugs there.

Second question: what error messages are useful?

From a users point of view, there really is only the question of unsupported vs. corrupted. Information like Component number mismatch provides no value to him at all.

From a develops point of view, I see the need to identify the exact location in the source code if something goes wrong (the function name is only half way).

Before I continue this with some concrete suggestions about how we can better achieve the above goals, I'll wait for some feedback on this 'analysis'.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by inspecting the ThrowXXX call sites and catch sites mentioned in the issue, including LJpegDecompressor::parseSOS and checkSupportInternal. Compare the exception classes and how callers use them, then document a concrete scope for improving types and messages; done means the proposed error-handling model is agreed and consistently applicable across the library.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.