Project

General

Profile

Task #655

Improve selection error reporting / switch to exceptions

Added by Teemu Murtola almost 9 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
selections
Target version:
Difficulty:
uncategorized
Close

Description

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).


Related issues

Related to GROMACS - Task #652: Change selection method implementation to use C++Blocked, need info
Related to GROMACS - Task #880: Exception handling in selection parsingClosed02/05/2012
Blocked by GROMACS - Task #985: Context information for exceptionsClosed08/02/2012

Associated revisions

Revision e77c7f06 (diff)
Added by Teemu Murtola almost 8 years ago

Use exceptions in selection compilation.

Converted parts that do not depend on selection evaluation.

Part of task #655.

Change-Id: Ie2fe5fdfebb47e5317eb0bf1e4b516b7f09de790

Revision 7d0ac5e5 (diff)
Added by Teemu Murtola almost 8 years ago

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.

Change-Id: I7f3fc705888a02a7cd99e46952129996ee49e4cc

Revision d10a77b6 (diff)
Added by Teemu Murtola almost 8 years ago

Added unit tests for some selection errors.

Part of tasks #651 and #655.

Change-Id: I3a6a9bb0e4e635eb93f5c5faa4e796415592b608

Revision 3ee636c5 (diff)
Added by Teemu Murtola almost 8 years ago

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.

Change-Id: If0d7b6c5ba36ad64f97ff2cfac8f7ab47b4d5a27

Revision 6e877567 (diff)
Added by Teemu Murtola about 7 years ago

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).

Related to #655 and #880.

Change-Id: I9e6ae98067f8fe96d39aea6df076e2675ecb8a1f

Revision ddaf0f3b (diff)
Added by Teemu Murtola about 7 years ago

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.

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
subsequent commits.

Related to #655 and #880.

Change-Id: Ib36c08bac1ab2c9818604e45e2eb14a90069efdb

Revision 6e52b492 (diff)
Added by Teemu Murtola about 7 years ago

C++ classes for selection parser symbol table.

Convert the implementation of selection parser symbol table to use C++
classes.
- 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.

Related to #655 and #880.

Change-Id: Iecb690fa2e413c9dc487aacffac62a248ea3867a

Revision 85b98012 (diff)
Added by Teemu Murtola about 7 years ago

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
selection parser.

Related to #655 and #880.

Change-Id: Ia4c477f1313b49cd1da3b0407bec6e7a1a8b9296

Revision 3e234326 (diff)
Added by Teemu Murtola about 7 years ago

C++ classes for selection parser values.

Converted t_selexpr_value to gmx::SelectionParserValue and introduced
smart pointers and standard containers to manage all memory related to
these objects.

Related to #655 and #880.

Change-Id: Ide2d9db80e89f36c94dccc25085d49959e4115a8

Revision 64a06b73 (diff)
Added by Teemu Murtola about 7 years ago

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.

Related to #655 and #880.

Change-Id: I377bac05b1bd990ad95b38887f0f7b825bf0a805

Revision 7ce47fc7 (diff)
Added by Teemu Murtola about 7 years ago

Remove remaining GMX_ERROR uses.

Replaced those still left in selection parsing code by exceptions or
assertions.

Related to #655 and #880.

Change-Id: I89bff0c03eb15176e699529888aaae532f0c4138

Revision a9beaee9 (diff)
Added by Teemu Murtola about 7 years ago

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
matching.

This change allows removing a few more direct prints to stderr (related
to #655).

Closes #938.

Change-Id: I7b998050c8b00b5f1229ed23a0a15685c514010f

Revision 6aeb717c (diff)
Added by Teemu Murtola about 6 years ago

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.

Related to #655 and #838.

Change-Id: I4ba23c6dd1005f61d50515e6857a5dc23fc1768a

Revision 907e5518 (diff)
Added by Teemu Murtola about 6 years ago

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
error.

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.

Related to #651 and #655.

Change-Id: I2318823b1be74775d0a0f8bd4a3c58b0aed1aba1

Revision 29a6c2ff (diff)
Added by Teemu Murtola over 5 years ago

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).

Fixes #1538, related to #655.

Change-Id: I1112b5ccae526cf3b8fd079bbabf8d7f61e52635

Revision bb406f89 (diff)
Added by Teemu Murtola almost 5 years ago

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
improved further.

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
non-interactive parsing.
- More consistent exception handling, e.g., in cases where an exception
is thrown during error handling.

Improve documentation for related functions.

Part of #655.

Change-Id: Ib276ce4e219692c9819e45a1637e3660d958803f

Revision 701533da (diff)
Added by Teemu Murtola almost 5 years ago

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.

Change-Id: I7b1a317e043c6ff791b25f5fa357d8166c577fae

Revision b39e1067 (diff)
Added by Teemu Murtola over 4 years ago

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).

Change-Id: If7bc1da91041232db23389c660cc2cd9c50addda

Revision ccc369c4 (diff)
Added by Teemu Murtola over 4 years ago

Remove old selection parser error handling code

Now that the parser fully uses exceptions for error handling, the old
mechanism can go, simplifying the code in a few places.

Clean-up for #655.

Change-Id: I92c472a5298e89dce80e022d1e2101d822d90045

History

#1 Updated by Teemu Murtola about 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.

#2 Updated by Teemu Murtola about 8 years ago

  • Status changed from New to In Progress
  • Assignee set to Teemu Murtola

#3 Updated by Teemu Murtola almost 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 about 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).

#5 Updated by Teemu Murtola over 5 years ago

  • Project changed from Next-generation analysis tools to GROMACS
  • Target version set to future

#6 Updated by Teemu Murtola over 5 years ago

  • Category set to selections

#7 Updated by Gerrit Code Review Bot over 5 years ago

Gerrit received a related patchset '1' for Issue #655.
Uploader: Teemu Murtola ()
Change-Id: I1112b5ccae526cf3b8fd079bbabf8d7f61e52635
Gerrit URL: https://gerrit.gromacs.org/3721

#8 Updated by Gerrit Code Review Bot almost 5 years ago

Gerrit received a related patchset '1' for Issue #655.
Uploader: Teemu Murtola ()
Change-Id: I7b1a317e043c6ff791b25f5fa357d8166c577fae
Gerrit URL: https://gerrit.gromacs.org/4209

#9 Updated by Gerrit Code Review Bot almost 5 years ago

Gerrit received a related patchset '1' for Issue #655.
Uploader: Teemu Murtola ()
Change-Id: If7bc1da91041232db23389c660cc2cd9c50addda
Gerrit URL: https://gerrit.gromacs.org/4210

#10 Updated by Teemu Murtola almost 5 years ago

  • Status changed from In Progress to Fix uploaded
  • Target version changed from future to 5.1

#11 Updated by Gerrit Code Review Bot almost 5 years ago

Gerrit received a related patchset '1' for Issue #655.
Uploader: Teemu Murtola ()
Change-Id: I92c472a5298e89dce80e022d1e2101d822d90045
Gerrit URL: https://gerrit.gromacs.org/4211

#12 Updated by Teemu Murtola over 4 years ago

  • Status changed from Fix uploaded to Closed

Also available in: Atom PDF