Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Handle file/index not found errors in reader classes gracefully instead of segfaulting! #337

Conversation

droazen
Copy link
Contributor

@droazen droazen commented Oct 30, 2014

This has been a common complaint against gamgee of late, and
with very good reason!

Resolves #336

…ad of segfaulting!

This has been a common complaint against gamgee of late, and
with very good reason!

Resolves #336
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.57%) when pulling a9b7f26 on dr_fix_additional_reader_segfaults_on_nonexistent_files into 3e4330f on master.


namespace gamgee {

/**
* @brief Exception for the case where there is an error opening a file for reading/writing
*/
class FileOpenException : public std::runtime_error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any good reason not to call this FileNotFoundException?

maybe archaic english concordance, or something I'm unaware of :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's for general file open errors, not ONLY file not found (htslib does not allow us to distinguish file not found from other types of file access errors)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question is does it really matter what the error was? we are not even distinguishing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important for the name of the exception not to be deceiving -- it shouldn't be called FileNotFoundException if it can be thrown when a file is present (but unreadable).

@MauricioCarneiro
Copy link
Contributor

After looking at the code, I'm okay with the 4 new exception classes. They are fine and if we choose to change them in the future, it is easy to do.

Please create the 2 tickets requested above ^^.

Other than that, this is good to merge. Good work.

MauricioCarneiro pushed a commit that referenced this pull request Oct 31, 2014
…segfaults_on_nonexistent_files

Handle file/index not found errors in reader classes gracefully instead of segfaulting!
@MauricioCarneiro MauricioCarneiro merged commit 0b18f36 into master Oct 31, 2014
@MauricioCarneiro MauricioCarneiro deleted the dr_fix_additional_reader_segfaults_on_nonexistent_files branch October 31, 2014 03:58
@jmthibault79
Copy link
Contributor

👍

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

Successfully merging this pull request may close these issues.

Fix segaults in all variant / sam readers on file/index not found
4 participants