Unidata / netcdf-cxx4

Official GitHub repository for netCDF-C++ libraries and utilities.
Other
124 stars 49 forks source link

Exception hierarchy #9

Closed jarlela closed 5 years ago

jarlela commented 10 years ago

The exception classes does not propagate all availabel error information from the netcdf-c api (e.g. returned system error codes are lost). This can easily be improved by including the netcdf-c return value as an error code. As a consequence the NcException constructor signatures must be modified.

Secondly, the netcdf-c api already includes a function to retreive an error message. Using nc_strerror in ncCheck ensures a consistent error message. This will also let any change/addition in the error codes of the netcdf-c api be reflected the NcException automatically.

Finally, in my opinion the hierarchy of exception classes seems a bit excessive. I think that structure in many cases will lead to many unnecessary catch blocks. I suggest that only a single exception class is used, but I suppose this is a bit of a discussion point. As long as the error code is included in NcException the user may choose to simply ignore the derived classes in any case.

slayoo commented 10 years ago

Hi,

Here's a related example from HDF5: http://www.hdfgroup.org/HDF5/doc/cpplus_RM/classH5_1_1Exception.html For some reason, they have also decided to have an exception-class hierarchy.

I'm not experienced in such designs. What, in practice, are the pros and cons of having such hierarchy? Here's a relevant discussion: http://stackoverflow.com/questions/9387377/how-to-design-exception-types-in-c but I don't find it conclusive in this context.

Sylwester

slayoo commented 10 years ago

Regarding 9f06cab & 39a24d7 - I fully agree! (in fact I suggested the strerror/NC_ISSYSERR fix in an e-mail to Lynton and Russ back in 2012 :) ).

jarlela commented 10 years ago

I probably should have made d6b629c a separate issue/pull request. I will revert the removal of exception classes included here, if that is preferred.

jarlela commented 10 years ago

I think the question of what is the best design for a hierarchy of exception classes is difficult. Having a large number of error classes for very specific errors such as the current code makes it very easy to catch specific errors. On the other hand, it will be inconvenient if the user wants to handle a number of these exceptions (but not all NcExceptions) in the same way. In that case a separate catch statement is needed for every exception class and there will be a lot of repeated code. With the proposed inclusion of the error code, a possible solution will be to catch the NcException and use the error code to select the relevant action.

With the inclusion of the error code in the NcException the large number of exception classes to specify the error is no longer needed. The question is then if they are still wanted. Pro:

An alternative to removing the exception classes could also be to make braoader exception classes and using the error code to specify the specific error. Pro:

russrew commented 10 years ago

It would be great if you could make d6b629c a separate issue/pull request, thanks! I will test and merge the resulting less controversial pull request quickly.

Your most recent comments on the issue of simplifying the hierarchy of exception classes are useful.

I'd like to get some feedback from current users of the netcdf-cxx4 library about how disruptive the resulting changes would be in comparison to the benefits of simplifying catching multiple error conditions. We're also having some internal discussions about this issue. I think it's relevant that Unidata hasn't announced any strong commitment to backwards compatibility of the C++ API, as we have for the C and Fortran APIs.

CLAassistant commented 5 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.