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

diff --git a/src/gromacs/selection/parsetree.cpp b/src/gromacs/selection/parsetree.cpp
index b268a57786c0fd95ebbfc9b05b0e62f8f738639d..a52346a951acfc9850b5c198c3fbd5c1821a1793 100644 (file)
--- a/src/gromacs/selection/parsetree.cpp
+++ b/src/gromacs/selection/parsetree.cpp
@@ -271,8 +271,20 @@ _gmx_selparser_error(yyscan_t scanner, const char *fmt, ...)
 }
 
 bool
-_gmx_selparser_handle_exception(yyscan_t scanner, const std::exception & /*ex*/)
+_gmx_selparser_handle_exception(yyscan_t scanner, const std::exception &ex)
 {
+    if (dynamic_cast<const gmx::UserInputError *>(&ex) != NULL)
+    {
+        // TODO: Consider whether also the non-interactive parser should
+        // postpone the exception such that the whole selection can be added as
+        // context.
+        if (_gmx_sel_is_lexer_interactive(scanner))
+        {
+            // TODO: Handle exceptions that printing the message may produce.
+            gmx::formatExceptionMessageToFile(stderr, ex);
+            return true;
+        }
+    }
     _gmx_sel_lexer_set_exception(scanner, boost::current_exception());
     return false;
 }

diff --git a/src/gromacs/utility/exceptions.cpp b/src/gromacs/utility/exceptions.cpp
index 1854b5e8203ac118c7032e400f6506fcf433be8c..c39ca3a7e8c9b3e94cff0fe23379fb71aeade885 100644 (file)
--- a/src/gromacs/utility/exceptions.cpp
+++ b/src/gromacs/utility/exceptions.cpp
@@ -222,54 +222,172 @@ int NotImplementedError::errorCode() const
 namespace
 {
 
+/*! \brief
+ * Abstracts actual output from the other logic in exception formatting.
+ *
+ * Object that implements this interface is passed to
+ * formatExceptionMessageInternal(), and is responsible for composing the
+ * output.  This allows using the same implementation of interpreting the
+ * exceptions while still supporting output to different formats (e.g., to a
+ * string or to stderr).
+ */
+class MessageWriterInterface
+{
+    public:
+        virtual ~MessageWriterInterface() {}
+
+        /*! \brief
+         * Writes a single line of text into the output.
+         *
+         * \param[in] text    Text to write on the line.
+         * \param[in] indent  Suggested number of spaces to indent the line.
+         */
+        virtual void writeLine(const char *text, int indent) = 0;
+        /*! \brief
+         * Writes information about a system error (errno-based).
+         *
+         * \param[in] errorNumber  errno value
+         * \param[in] funcName     Name of the system call (can be NULL).
+         * \param[in] indent       Suggested number of spaces to indent the output.
+         */
+        virtual void writeErrNoInfo(int errorNumber, const char *funcName,
+                                    int indent) = 0;
+};
+
+/*! \brief
+ * Exception information writer for cases where exceptions should be avoided.
+ *
+ * Formats the messages into the provided FILE handle without checking for
+ * errors in fprintf() calls.
+ */
+class MessageWriterFileNoThrow : public MessageWriterInterface
+{
+    public:
+        //! Initializes a writer that writes to the given file handle.
+        explicit MessageWriterFileNoThrow(FILE *fp) : fp_(fp) {}
+
+        virtual void writeLine(const char *text, int indent)
+        {
+            internal::printFatalErrorMessageLine(fp_, text, indent);
+        }
+        virtual void writeErrNoInfo(int errorNumber, const char *funcName,
+                                    int indent)
+        {
+            std::fprintf(fp_, "%*sReason: %s\n", indent, "",
+                         std::strerror(errorNumber));
+            if (funcName != NULL)
+            {
+                std::fprintf(fp_, "%*s(call to %s() returned error code %d)\n",
+                             indent, "", funcName, errorNumber);
+            }
+        }
+
+    private:
+        FILE                   *fp_;
+};
+
+/*! \brief
+ * Exception information writer to format into an std::string.
+ */
+class MessageWriterString : public MessageWriterInterface
+{
+    public:
+        //! Post-processes the output string to not end in a line feed.
+        void removeTerminatingLineFeed()
+        {
+            if (result_.size() > 0U)
+            {
+                result_.erase(result_.size() - 1);
+            }
+        }
+        //! Returns the constructed string.
+        const std::string &result() const { return result_; }
+
+        virtual void writeLine(const char *text, int indent)
+        {
+            result_.append(indent, ' ');
+            result_.append(text);
+            result_.append("\n");
+        }
+        virtual void writeErrNoInfo(int errorNumber, const char *funcName,
+                                    int indent)
+        {
+            writeLine(formatString("Reason: %s", std::strerror(errorNumber)).c_str(),
+                      indent);
+            if (funcName != NULL)
+            {
+                writeLine(formatString("(call to %s() returned error code %d)",
+                                       funcName, errorNumber).c_str(),
+                          indent);
+            }
+        }
+
+    private:
+        std::string             result_;
+};
+
 /*! \brief
  * Prints error information for an exception object.
  *
- * \param[in] fp      File to write the information out to (typically stderr).
+ * \param[in] writer  Writer to write out the information.
  * \param[in] ex      Exception object to print.
  * \param[in] indent  Indentation for the information.
  *
  * If the exception contains nested exceptions, information from them is
  * recursively printed.
+ *
+ * Does not throw unless the writer throws.
  */
-void printExceptionMessage(FILE *fp, const std::exception &ex, int indent)
+void formatExceptionMessageInternal(MessageWriterInterface *writer,
+                                    const std::exception &ex, int indent)
 {
     const boost::exception *boostEx = dynamic_cast<const boost::exception *>(&ex);
     if (boostEx != NULL)
     {
+        // TODO: Add an option to print this information for the tests
+        // const char *const *funcPtr =
+        //     boost::get_error_info<boost::throw_function>(*boostEx);
+        // const char *const *filePtr =
+        //     boost::get_error_info<boost::throw_file>(*boostEx);
+        // const int         *linePtr =
+        //     boost::get_error_info<boost::throw_line>(*boostEx);
+
+        // std::string        result;
+        // if (filePtr != NULL && linePtr != NULL)
+        // {
+        //     result = formatString("%s:%d: %s\n", *filePtr, *linePtr,
+        //                           funcPtr != NULL ? *funcPtr : "");
+        // }
+
         // TODO: Remove duplicate context if present in multiple nested exceptions.
         const ErrorMessage *msg = boost::get_error_info<errinfo_message>(*boostEx);
         if (msg != NULL)
         {
             while (msg != NULL && msg->isContext())
             {
-                internal::printFatalErrorMessageLine(fp, msg->text().c_str(), indent*2);
+                writer->writeLine(msg->text().c_str(), indent*2);
                 ++indent;
                 msg = &msg->child();
             }
             if (msg != NULL && !msg->text().empty())
             {
-                internal::printFatalErrorMessageLine(fp, msg->text().c_str(), indent*2);
+                writer->writeLine(msg->text().c_str(), indent*2);
             }
         }
         else
         {
-            internal::printFatalErrorMessageLine(fp, ex.what(), indent);
+            writer->writeLine(ex.what(), indent*2);
         }
 
         const int *errorNumber
             = boost::get_error_info<boost::errinfo_errno>(*boostEx);
         if (errorNumber != NULL)
         {
-            std::fprintf(fp, "%*sReason: %s\n", (indent+1)*2, "",
-                         std::strerror(*errorNumber));
             const char * const *funcName
                 = boost::get_error_info<boost::errinfo_api_function>(*boostEx);
-            if (funcName != NULL)
-            {
-                std::fprintf(fp, "%*s(call to %s() returned error code %d)\n",
-                             (indent+1)*2, "", *funcName, *errorNumber);
-            }
+            writer->writeErrNoInfo(*errorNumber,
+                                   funcName != NULL ? *funcName : NULL,
+                                   (indent+1)*2);
         }
 
         // TODO: Treat also boost::nested_exception (not currently used, though)
@@ -287,14 +405,14 @@ void printExceptionMessage(FILE *fp, const std::exception &ex, int indent)
                 }
                 catch (const std::exception &nestedEx)
                 {
-                    printExceptionMessage(fp, nestedEx, indent + 1);
+                    formatExceptionMessageInternal(writer, nestedEx, indent + 1);
                 }
             }
         }
     }
     else
     {
-        internal::printFatalErrorMessageLine(fp, ex.what(), indent);
+        writer->writeLine(ex.what(), indent*2);
     }
 }
 
@@ -353,100 +471,23 @@ void printFatalErrorMessage(FILE *fp, const std::exception &ex)
     {
         std::fprintf(fp, "(exception type: %s)\n", typeid(ex).name());
     }
-    printExceptionMessage(fp, ex, 0);
+    MessageWriterFileNoThrow writer(fp);
+    formatExceptionMessageInternal(&writer, ex, 0);
     internal::printFatalErrorFooter(fp);
 }
 
-std::string formatException(const std::exception &ex)
+std::string formatExceptionMessageToString(const std::exception &ex)
 {
-    // TODO: It would be nicer to not duplicate the logic from
-    // printExceptionMessage().
-    const boost::exception *boostEx = dynamic_cast<const boost::exception *>(&ex);
-    if (boostEx != NULL)
-    {
-        const char *const *funcPtr =
-            boost::get_error_info<boost::throw_function>(*boostEx);
-        const char *const *filePtr =
-            boost::get_error_info<boost::throw_file>(*boostEx);
-        const int         *linePtr =
-            boost::get_error_info<boost::throw_line>(*boostEx);
-
-        std::string        result;
-        if (filePtr != NULL && linePtr != NULL)
-        {
-            result = formatString("%s:%d: %s\n", *filePtr, *linePtr,
-                                  funcPtr != NULL ? *funcPtr : "");
-        }
-
-        // TODO: Remove duplicate context if present in multiple nested exceptions.
-        const ErrorMessage *msg =
-            boost::get_error_info<errinfo_message>(*boostEx);
-        if (msg != NULL)
-        {
-            while (msg != NULL && msg->isContext())
-            {
-                result.append(msg->text());
-                result.append("\n");
-                msg = &msg->child();
-            }
-            if (msg != NULL && !msg->text().empty())
-            {
-                result.append(msg->text());
-                result.append("\n");
-            }
-        }
-        else
-        {
-            result.append(ex.what());
-            result.append("\n");
-        }
-
-        const int *errorNumber
-            = boost::get_error_info<boost::errinfo_errno>(*boostEx);
-        if (errorNumber != NULL)
-        {
-            result.append(formatString("Reason: %s\n",
-                                       std::strerror(*errorNumber)));
-            const char * const *funcName
-                = boost::get_error_info<boost::errinfo_api_function>(*boostEx);
-            if (funcName != NULL)
-            {
-                result.append(formatString("(call to %s() returned error code %d)\n",
-                                           *funcName, *errorNumber));
-            }
-        }
-
-        // TODO: Treat also boost::nested_exception (not currently used, though)
+    MessageWriterString writer;
+    formatExceptionMessageInternal(&writer, ex, 0);
+    writer.removeTerminatingLineFeed();
+    return writer.result();
+}
 
-        const internal::NestedExceptionList *nested
-            = boost::get_error_info<errinfo_nested_exceptions>(*boostEx);
-        if (nested != NULL)
-        {
-            internal::NestedExceptionList::const_iterator ni;
-            for (ni = nested->begin(); ni != nested->end(); ++ni)
-            {
-                try
-                {
-                    rethrow_exception(*ni);
-                }
-                catch (const std::exception &nestedEx)
-                {
-                    result.append(formatException(nestedEx));
-                    result.append("\n");
-                }
-            }
-        }
-        // Remove terminating line feed.
-        if (result.size() > 0U)
-        {
-            result.erase(result.size() - 1);
-        }
-        return result;
-    }
-    else
-    {
-        return ex.what();
-    }
+void formatExceptionMessageToFile(FILE *fp, const std::exception &ex)
+{
+    MessageWriterFileNoThrow writer(fp);
+    formatExceptionMessageInternal(&writer, ex, 0);
 }
 
 } // namespace gmx

diff --git a/src/gromacs/utility/exceptions.h b/src/gromacs/utility/exceptions.h
index d6826e0c749a12ab7ddb1bf6a88cffadd2d59ea2..a0aacfe3fd30b48f7ebdcdccfae027168f2c8454 100644 (file)
--- a/src/gromacs/utility/exceptions.h
+++ b/src/gromacs/utility/exceptions.h
@@ -84,7 +84,8 @@ typedef std::vector<boost::exception_ptr> NestedExceptionList;
  * \todo
  * With the exception of the reason string, information added with this class
  * is not currently accessible through any public API, except for calling
- * printFatalErrorMessage().  This is not implemented as there is no current
+ * printFatalErrorMessage(), formatExceptionMessageToString() or
+ * formatExceptionMessageToFile().  This is not implemented as there is not yet
  * need for it, and it is not clear what would be the best alternative for the
  * access.  It should be possible to refactor the internal implementation to
  * suit the needs of such external access without requiring changes in code
@@ -185,7 +186,8 @@ class GromacsException : public std::exception, public boost::exception
          * \todo
          * The added information is currently not accessible through what(),
          * nor through any other means except for calling
-         * printFatalErrorMessage(). See ExceptionInitializer for more
+         * printFatalErrorMessage(), formatExceptionMessageToString() or
+         * formatExceptionMessageToFile(). See ExceptionInitializer for more
          * discussion.
          */
         void prependContext(const std::string &context);
@@ -417,12 +419,16 @@ void printFatalErrorMessage(FILE *fp, const std::exception &ex);
  * \param[in] ex  Exception to format.
  * \returns   Formatted string containing details of \p ex.
  * \throws    std::bad_alloc if out of memory.
+ */
+std::string formatExceptionMessageToString(const std::exception &ex);
+/*! \brief
+ * Formats an error message for reporting an exception.
  *
- * Currently, the output format is useful mainly for tests and debugging
- * purposes; additional flags for controlling the format can be added if other
- * uses for the function arise.
+ * \param     fp  File to write the message to.
+ * \param[in] ex  Exception to format.
+ * \throws    std::bad_alloc if out of memory.
  */
-std::string formatException(const std::exception &ex);
+void formatExceptionMessageToFile(FILE *fp, const std::exception &ex);
 
 /*! \brief
  * Converts an exception into a return code.

diff --git a/src/testutils/testasserts.h b/src/testutils/testasserts.h
index 664973f182e7b59b0ad6306aaf5d1f915bc790b0..8eb6af261e42a6ee1e079c4d60edc25d78a99d6d 100644 (file)
--- a/src/testutils/testasserts.h
+++ b/src/testutils/testasserts.h
@@ -92,7 +92,7 @@ namespace test
         catch (std::exception const &ex) { \
             gmx_ar << "Expected: " #statement " throws an exception of type " \
             << #expected_exception ".\n  Actual: it throws a different type.\n" \
-            << "Exception details:\n" << ::gmx::formatException(ex); \
+            << "Exception details:\n" << ::gmx::formatExceptionMessageToString(ex); \
             goto GTEST_CONCAT_TOKEN_(gmx_label_testthrow_, __LINE__); \
         } \
         catch (...) { \
@@ -128,7 +128,7 @@ namespace test
         catch (std::exception const &ex) { \
             gmx_ar << "Expected: " #statement " doesn't throw an exception.\n" \
             << "  Actual: it throws.\n" \
-            << "Exception details:\n" << ::gmx::formatException(ex); \
+            << "Exception details:\n" << ::gmx::formatExceptionMessageToString(ex); \
             goto GTEST_CONCAT_TOKEN_(gmx_label_testnothrow_, __LINE__); \
         } \
         catch (...) { \