Improve selection error reporting / switch to exceptions
Substantial improvements in the error messages was done in d3cee92, but as stated there, there are still cases that print directly to stderr (in particular, all errors detected during evaluation or selection method registration), and the error messages could be made clearer. Right now, even an expert cannot always tell immediately from the error message what is the reason. One thing that would help, but would also take some effort to implement, would be to report a wider context for the error (e.g., by including the erroneous part of the selection in the error message).
Use exceptions in selection compilation.
Converted parts that do not depend on selection evaluation.
Part of task #655.
Use exceptions in selection evaluation.
Converted also the rest of selection compilation, because it internally
uses the evaluation engine.
Will add some unit tests for error conditions in a subsequent commit.
Part of task #655.
Removed some direct output to stderr.
Functions that used to print information directly to stderr now take a
FILE object such that the caller can decide where the information goes.
The caller for now still always prints to stderr, but this reduces the
number of direct references to stderr. Remaining references to stderr in
the selection module are either for 1) part of interactive input, 2)
debug output, or 3) rare error conditions that require some thought on
how to handle.
Related to issue #655.
Remove most uses of t_selelem::refcount.
This is a prerequisite for using smart pointers to manage the selection
evaluation tree, as it's difficult to keep a similar reference count
up-to-date and (or use pointer reference counts for the purposes it is
currently used for).
Use smart pointers to manage the selection tree.
Changed t_selelem to gmx::SelectionTreeElement and t_selelem * to
gmx::SelectionTreeElementPointer (or a const reference). The latter is
a boost::shared_ptr because there may be multiple references to some
elements in the evaluation tree. Some exceptions to the above rule are
present, because it was not straightforward to change those parts.
Prerequisite for using exceptions throughout the selection code, which
in turn is a prerequisite for many tasks that would introduce more C++
There are some parts that are not yet exception-safe, but may throw
std::bad_alloc as the result of these changes. Will fix those in
C++ classes for selection parser symbol table.
Convert the implementation of selection parser symbol table to use C++
- Removes a few exception safety issues that were present in the
initialization, as well as direct prints to stderr.
- Documentation now follows the new Doxygen layout.
- Removes some underscore-prefixed global names.
C++ classes for selection parser parameters.
Converted t_selexpr_param to gmx::SelectionParserParameter and
introduced smart pointers and standard containers to manage all memory
related to these objects. Solves several exception safety issues in the
Handle SelectionParserParameter as a value type.
Now that SelectionParserValue is also a C++ object, it is
straightforward to treat SelectionParserParameter as a (movable, but not
copyable) value type instead of storing it through a smart pointer in a
container. Leads to more consistent and less complex code.
Add syntax to force selection string matching mode.
The selection syntax for string keyword matching no longer depends on
whether regular expression support is available. Instead, an error is
now given if the string looks like a regexp (the logic for the deduction
is not changed), but regexp support is not available. Added syntax to
force the string matching to use either literal, wildcard, or regexp
This change allows removing a few more direct prints to stderr (related
Treat exceptions better in interactive selections.
If a user input error occurs in an interactive selection parser, the
message is now printed and parsing continues, like it used to work
before exceptions were introduced here. Although most code still does
not use exceptions for user error reporting, this makes the code more
future-proof (and a subsequent commit will add some exceptions that take
advantage of this).
Refactored exception message formatting to support this, removing a TODO
related to duplicated code in the process.
Add check for unsorted index groups in selections.
Selections did not work properly if an index group was used that
did not have the atom indices in strictly ascending order.
Added a check for this, and tests that check that this now gives an
Refactored the way index group references are resolved such that the
logic and the related error messages are now only in one place:
selelem.cpp. This resolves TODOs about improving the error messages.
Also made this piece of code to use exceptions for error reporting.
Remove some hardcoded selection string buffers
This removes some arbitrary limitations on the length of the input
string. The error handling would still be in the need of a bigger
overhaul (all of these were in the error handling parts).
Improve selection parsing error reporting
Now the selection parser tracks the part of selection text that
contributes to the current parsing context, and uses that for more
useful error messages. At least in some cases, there is now also
context information for syntax errors, although this could likely be
Exception handling during selection parsing is also improved:
- The above context is now added to the exception, similar to what the
old reporting mechanism does.
- The full selection is added to the exception as context, also for
- More consistent exception handling, e.g., in cases where an exception
is thrown during error handling.
Improve documentation for related functions.
Part of #655.
Track locations for selection parsing products
SelectionTreeElement and all intermediate structures used for selection
method parameter parsing now track the location in the input string.
By itself, this change does nothing, but it enables the subsequent
change that switches to exceptions to use this information for much
better context information in the error messages.
The location is kept as a SelectionLocation instead of as a string to
avoid spending a lot of memory for these strings with complex
selections: in general, each node in the tree contains the concatenation
of the strings from its child nodes, and there can be quite a few nodes
at the top that all contain have the full selection as their location.
Related to #655.
Improve selection error messages (use exceptions)
Make all selection parsing error handling use exceptions for error
reporting, and improve the understandability and context-sensitivity of
the messages. Make the code in params.cpp exception-safe.
Resolves #655 (except for some pending cleanup of now-unused code).
#1 Updated by Teemu Murtola over 8 years ago
- Subject changed from Improve error reporting in selection library to Improve selection error reporting / switch to exceptions
The cases that print directly to stderr should be possible to get rid of by changing selection evaluation (and compilation) to use exceptions for error reporting. This is also a prerequisite for task #652, as well as some other clean-up, as it is much cleaner to use C++ in functions called during selection evaluation if exceptions don't need to be translated to return codes and then back to exceptions. With this change, all of the public C++ interfaces would then use exceptions for error reporting: currently compilation and evaluation still use return codes.
For selection method registration, the all the checks will need to be fundamentally changed (most will become part of the options module or a class that derives from AbstractOptionStorage) as part of task #652, so there is no need to clean up those at this point.
#3 Updated by Teemu Murtola over 8 years ago
The series of commits now pending review in Gerrit (from Ie2fe5fdf to If0d7b6c5) complete the transition to exceptions. Most direct prints to stderr are now also gone. The error messages could still be improved, but that requires some careful thought on what would be good and what can be implemented with reasonable effort.
#4 Updated by Teemu Murtola over 7 years ago
Now that #880 is solved, it should be possible to convert also the selection parser to use exceptions internally for error handling. I already have a local copy where I have changed the t_selelem tree such that it is managed using smart pointers, which was by far the biggest issue in making the code exception-safe. But in order to convert the error reporting to exceptions, #985 needs to be solved first. This should allow removing the remaining direct prints to stderr (except for the selection method registration, which is not an issue at this moment).