zorba-coders team mailing list archive

["Paul J. Lucas" <paul@xxxxxxxxxxxxx>]

Paul J. Lucas has proposed merging lp:~zorba-coders/zorba/bug-1103819 into lp:zorba.

Commit message:
Reworked some catch clauses.

Requested reviews:
  Paul J. Lucas (paul-lucas)
Related bugs:
  Bug #1103819 in Zorba: "Use of "catch (...)" should be reduced or mitigated"
  https://bugs.launchpad.net/zorba/+bug/1103819
  Bug #1106581 in Zorba: "Use of "throw 0" as flow-control"
  https://bugs.launchpad.net/zorba/+bug/1106581
  Bug #1106586 in Zorba: "Use of "catch (...)" swallows exceptions"
  https://bugs.launchpad.net/zorba/+bug/1106586

For more details, see:
https://code.launchpad.net/~zorba-coders/zorba/bug-1103819/+merge/146312

Reworked some catch clauses.
-- 
https://code.launchpad.net/~zorba-coders/zorba/bug-1103819/+merge/146312
Your team Zorba Coders is subscribed to branch lp:zorba.

=== modified file 'src/api/xqueryimpl.cpp'
--- src/api/xqueryimpl.cpp	2013-01-08 08:34:08 +0000
+++ src/api/xqueryimpl.cpp	2013-02-03 18:00:35 +0000
@@ -79,6 +79,7 @@
 namespace zorba
 {
 
+#define QUERY_TRY try
 
 #define QUERY_CATCH                                                 \
   catch (ZorbaException const& e)                                   \
@@ -941,7 +942,7 @@
 {
   SYNC_CODE(AutoMutex lock(&theMutex);)
 
-  try
+  QUERY_TRY
   {
     checkNotClosed();
     checkCompiled();
@@ -964,22 +965,7 @@
 
     return true;
   }
-  catch (ZorbaException const& e)
-  {
-    ZorbaImpl::notifyError(theDiagnosticHandler, e);
-  }
-  catch (FlowCtlException const&)
-  {
-    ZorbaImpl::notifyError(theDiagnosticHandler, "User interrupt");
-  }
-  catch (std::exception const& e)
-  {
-    ZorbaImpl::notifyError(theDiagnosticHandler, e.what());
-  }
-  catch (...)
-  {
-    ZorbaImpl::notifyError(theDiagnosticHandler);
-  }
+  QUERY_CATCH
 
   return false;
 }

=== modified file 'src/capi/cdynamic_context.cpp'
--- src/capi/cdynamic_context.cpp	2012-09-19 21:16:15 +0000
+++ src/capi/cdynamic_context.cpp	2013-02-03 18:00:35 +0000
@@ -271,10 +271,7 @@
   CDynamicContext::free(XQC_DynamicContext* context)
   {
     try {
-      CDynamicContext* me = CDynamicContext::get(context);
-      delete me;
-    } catch (ZorbaException const&) { 
-      assert(false);
+      delete CDynamicContext::get(context);
     } catch (...) { 
       assert(false);
     }

=== modified file 'src/capi/cexpression.cpp'
--- src/capi/cexpression.cpp	2012-09-19 21:16:15 +0000
+++ src/capi/cexpression.cpp	2013-02-03 18:00:35 +0000
@@ -227,10 +227,7 @@
   CExpression::free(XQC_Expression* expr)
   {
     try {
-      CExpression* me = CExpression::get(expr);
-      delete me;
-    } catch (ZorbaException const&) {
-      assert(false);
+      delete CExpression::get(expr);
     } catch (...) {
       assert(false);
     }

=== modified file 'src/capi/cimplementation.cpp'
--- src/capi/cimplementation.cpp	2012-09-19 21:16:15 +0000
+++ src/capi/cimplementation.cpp	2013-02-03 18:00:35 +0000
@@ -507,10 +507,7 @@
   CImplementation::free(XQC_Implementation* impl)
   {
     try {
-      CImplementation* me = CImplementation::get(impl);
-      delete me;
-    } catch (ZorbaException const&) {
-      assert(false);
+      delete CImplementation::get(impl);
     } catch (...) {
       assert(false);
     }

=== modified file 'src/capi/csequence.cpp'
--- src/capi/csequence.cpp	2012-09-19 21:16:15 +0000
+++ src/capi/csequence.cpp	2013-02-03 18:00:35 +0000
@@ -492,10 +492,7 @@
 CSequence::free(XQC_Sequence* seq)
 {
   try {
-    CSequence* me = CSequence::get(seq);
-    delete me;
-  } catch (ZorbaException const&) {
-    assert(false);
+    delete CSequence::get(seq);
   } catch (...) {
     assert(false);
   }

=== modified file 'src/capi/cstatic_context.cpp'
--- src/capi/cstatic_context.cpp	2012-09-19 21:16:15 +0000
+++ src/capi/cstatic_context.cpp	2013-02-03 18:00:35 +0000
@@ -666,10 +666,7 @@
   CStaticContext::free(XQC_StaticContext* context)
   {
     try {
-      CStaticContext* me = CStaticContext::get(context);
-      delete me;
-    } catch (ZorbaException const&) { 
-      assert(false);
+      delete CStaticContext::get(context);
     } catch (...) { 
       assert(false);
     }

=== modified file 'src/context/default_url_resolvers.cpp'
--- src/context/default_url_resolvers.cpp	2013-01-24 12:11:43 +0000
+++ src/context/default_url_resolvers.cpp	2013-02-03 18:00:35 +0000
@@ -21,6 +21,7 @@
 #include "util/uri_util.h"
 #include "util/http_util.h"
 #include "util/fs_util.h"
+#include "util/string_util.h"
 #include "store/api/store.h"
 #include "store/api/item_factory.h"
 #include "store/api/collection.h"
@@ -93,6 +94,11 @@
         new StreamResource(&(lStream->getStream()), lStream->getStreamReleaser());
     lStream->setStreamReleaser(nullptr);
     return lResource;
+  } catch (std::exception const &e) {
+    std::string const msg(
+      BUILD_STRING( "Could not create stream resource: ", e.what() )
+    );
+    throw os_error::exception( "", aUrl.c_str(), msg.c_str() );
   } catch (...) {
     throw os_error::exception("", aUrl.c_str(), "Could not create stream resource");
   }

=== modified file 'src/runtime/debug/debug_iterator_impl.cpp'
--- src/runtime/debug/debug_iterator_impl.cpp	2012-09-19 21:16:15 +0000
+++ src/runtime/debug/debug_iterator_impl.cpp	2013-02-03 18:00:35 +0000
@@ -205,25 +205,18 @@
   if (lCommons->canBreak() && (lCommons->mustBreak(lCause) ||
       lCommons->hasToBreakAt(loc) || lCommons->hasToBreakAt(this)))
   {
-    try
-    {
-      lCause = lCause == 0 ? CAUSE_BREAKPOINT : lCause;
-      
-      // tell everybody that we are the iterator who suspended
-      //lCommons->setCurrentIterator(this);
-      lCommons->setCurrentStaticContext(getStaticContext());
-      lCommons->setCurrentDynamicContext(planState->theLocalDynCtx);
-      lCommons->setBreak(false);
-      lCommons->setPlanState(planState);
+    lCause = lCause == 0 ? CAUSE_BREAKPOINT : lCause;
+    
+    // tell everybody that we are the iterator who suspended
+    //lCommons->setCurrentIterator(this);
+    lCommons->setCurrentStaticContext(getStaticContext());
+    lCommons->setCurrentDynamicContext(planState->theLocalDynCtx);
+    lCommons->setBreak(false);
+    lCommons->setPlanState(planState);
 
-      // suspend
-      lCommons->getRuntime()->suspendRuntime(loc, lCause);
-      lCause = 0;
-    }
-    catch (...)
-    {
-      throw;
-    }
+    // suspend
+    lCommons->getRuntime()->suspendRuntime(loc, lCause);
+    lCause = 0;
   }
 }
 #endif // ZORBA_WITH_DEBUGGER

=== modified file 'src/runtime/indexing/doc_indexer.cpp'
--- src/runtime/indexing/doc_indexer.cpp	2012-09-19 21:16:15 +0000
+++ src/runtime/indexing/doc_indexer.cpp	2013-02-03 18:00:35 +0000
@@ -150,9 +150,7 @@
       }
       catch(...)
       {
-        if (key != NULL)
-          delete key;
-
+        delete key;
         throw;
       }
     }

=== modified file 'src/store/naive/loader_dtd.cpp'
--- src/store/naive/loader_dtd.cpp	2013-01-30 02:18:08 +0000
+++ src/store/naive/loader_dtd.cpp	2013-02-03 18:00:35 +0000
@@ -384,6 +384,9 @@
   }
   catch (...)
   {
+    // catch-TODO: "throw 0" and "catch(...)" are being used here as "flow
+    // control" -- not an appropriate use of C++ exceptions.  The catch here
+    // will silently swallow all other exceptions as well -- not good.
     abortload();
     thePathStack.clear();
     return NULL;
@@ -760,6 +763,14 @@
       )
     );
   }
+  catch (std::exception const &e)
+  {
+    theXQueryDiagnostics->add_error(
+      NEW_ZORBA_EXCEPTION(
+        zerr::ZSTR0020_LOADER_IO_ERROR, ERROR_PARAMS( e.what() )
+      )
+    );
+  }
   catch (...)
   {
     theXQueryDiagnostics->add_error(
@@ -1092,6 +1103,14 @@
   {
     loader.theXQueryDiagnostics->add_error( e );
   }
+  catch (std::exception const &e)
+  {
+    loader.theXQueryDiagnostics->add_error(
+      NEW_ZORBA_EXCEPTION(
+        zerr::ZXQP0003_INTERNAL_ERROR, ERROR_PARAMS( e.what() )
+      )
+    );
+  }
   catch (...)
   {
     loader.theXQueryDiagnostics->add_error(
@@ -1174,6 +1193,14 @@
   {
     loader.theXQueryDiagnostics->add_error( e );
   }
+  catch (std::exception const &e)
+  {
+    loader.theXQueryDiagnostics->add_error(
+      NEW_ZORBA_EXCEPTION(
+        zerr::ZXQP0003_INTERNAL_ERROR, ERROR_PARAMS( e.what() )
+      )
+    );
+  }
   catch (...)
   {
     loader.theXQueryDiagnostics->add_error(
@@ -1468,6 +1495,14 @@
   {
     loader.theXQueryDiagnostics->add_error( e );
   }
+  catch (std::exception const &e)
+  {
+    loader.theXQueryDiagnostics->add_error(
+      NEW_ZORBA_EXCEPTION(
+        zerr::ZXQP0003_INTERNAL_ERROR, ERROR_PARAMS( e.what() )
+      )
+    );
+  }
   catch (...)
   {
     loader.theXQueryDiagnostics->
@@ -1600,6 +1635,14 @@
   {
     loader.theXQueryDiagnostics->add_error( e );
   }
+  catch (std::exception const &e)
+  {
+    loader.theXQueryDiagnostics->add_error(
+      NEW_ZORBA_EXCEPTION(
+        zerr::ZXQP0003_INTERNAL_ERROR, ERROR_PARAMS( e.what() )
+      )
+    );
+  }
   catch (...)
   {
     loader.theXQueryDiagnostics->add_error(
@@ -1651,6 +1694,14 @@
   {
     loader.theXQueryDiagnostics->add_error( e );
   }
+  catch (std::exception const &e)
+  {
+    loader.theXQueryDiagnostics->add_error(
+      NEW_ZORBA_EXCEPTION(
+        zerr::ZXQP0003_INTERNAL_ERROR, ERROR_PARAMS( e.what() )
+      )
+    );
+  }
   catch (...)
   {
     loader.theXQueryDiagnostics->add_error(
@@ -1704,6 +1755,14 @@
   {
     loader.theXQueryDiagnostics->add_error( e );
   }
+  catch (std::exception const &e)
+  {
+    loader.theXQueryDiagnostics->add_error(
+      NEW_ZORBA_EXCEPTION(
+        zerr::ZXQP0003_INTERNAL_ERROR, ERROR_PARAMS( e.what() )
+      )
+    );
+  }
   catch (...)
   {
     loader.theXQueryDiagnostics->add_error(
@@ -1754,6 +1813,14 @@
   {
     loader.theXQueryDiagnostics->add_error( e );
   }
+  catch (std::exception const &e)
+  {
+    loader.theXQueryDiagnostics->add_error(
+      NEW_ZORBA_EXCEPTION(
+        zerr::ZXQP0003_INTERNAL_ERROR, ERROR_PARAMS( e.what() )
+      )
+    );
+  }
   catch (...)
   {
     loader.theXQueryDiagnostics->add_error(
@@ -1797,6 +1864,14 @@
   {
     loader.theXQueryDiagnostics->add_error( e );
   }
+  catch (std::exception const &e)
+  {
+    loader.theXQueryDiagnostics->add_error(
+      NEW_ZORBA_EXCEPTION(
+        zerr::ZXQP0003_INTERNAL_ERROR, ERROR_PARAMS( e.what() )
+      )
+    );
+  }
   catch (...)
   {
     loader.theXQueryDiagnostics->add_error(

=== modified file 'src/store/naive/loader_fast.cpp'
--- src/store/naive/loader_fast.cpp	2013-01-21 03:22:59 +0000
+++ src/store/naive/loader_fast.cpp	2013-02-03 18:00:35 +0000
@@ -71,6 +71,25 @@
       return; \
   } while (0);
 
+#define LOADER_TRY try
+
+#define LOADER_CATCH                                            \
+  catch ( ZorbaException const &e ) {                           \
+    loader.theXQueryDiagnostics->add_error( e );                \
+  }                                                             \
+  catch ( std::exception const &e ) {                           \
+    loader.theXQueryDiagnostics->add_error(                     \
+      NEW_ZORBA_EXCEPTION(                                      \
+        zerr::ZXQP0003_INTERNAL_ERROR, ERROR_PARAMS( e.what() ) \
+      )                                                         \
+    );                                                          \
+  }                                                             \
+  catch ( ... ) {                                               \
+    loader.theXQueryDiagnostics->add_error(                     \
+      NEW_ZORBA_EXCEPTION( zerr::ZXQP0003_INTERNAL_ERROR )      \
+    );                                                          \
+  }
+
 
 /*******************************************************************************
 
@@ -327,7 +346,8 @@
 
     return stream.gcount();
   }
-  catch (std::iostream::failure e)
+  // catch-TODO: should ZorbaException be caught seperately here?
+  catch (std::exception const &e)
   {
     theXQueryDiagnostics->add_error(
       NEW_ZORBA_EXCEPTION(
@@ -443,7 +463,7 @@
   catch(...)
   {
     abortload();
-    return NULL;
+    return NULL; // catch-TODO: "throw;" here?
   }
 
   bool ok = ctxt->wellFormed != 0;
@@ -498,7 +518,7 @@
   FastXmlLoader& loader = *(static_cast<FastXmlLoader *>(ctx));
   ZORBA_LOADER_CHECK_ERROR(loader);
   
-  try
+  LOADER_TRY
   {
     DocumentNode* docNode = GET_STORE().getNodeFactory().createDocumentNode();
 
@@ -523,15 +543,7 @@
 
     LOADER_TRACE1("Start Doc Node = " << docNode);
   }
-  catch (ZorbaException const& e)
-  {
-    loader.theXQueryDiagnostics->add_error( e );
-  }
-  catch (...)
-  {
-    loader.theXQueryDiagnostics->
-    add_error(NEW_ZORBA_EXCEPTION(zerr::ZXQP0003_INTERNAL_ERROR));
-  }
+  LOADER_CATCH
 }
 
 
@@ -554,7 +566,7 @@
   DocumentNode* docNode;
   XmlNode* currChild;
   
-  try
+  LOADER_TRY
   {
     // This check is required because it is possible (in case of mal-formed doc)
     // that libXml calls endDocument() without having called startDocument().
@@ -608,16 +620,7 @@
 
     LOADER_TRACE2("End Doc Node = " << docNode);
   }
-  catch (ZorbaException const& e)
-  {
-    loader.theXQueryDiagnostics->add_error( e );
-  }
-  catch (...)
-  {
-    loader.theXQueryDiagnostics->add_error(
-      NEW_ZORBA_EXCEPTION( zerr::ZXQP0003_INTERNAL_ERROR )
-    );
-  }
+  LOADER_CATCH
 }
 
 
@@ -658,7 +661,7 @@
   zorba::Stack<PathStepInfo>& pathStack = loader.thePathStack;
   zstring baseUri;
 
-  try
+  LOADER_TRY
   {
     csize numAttributes = static_cast<csize>(numAttrs);
     csize numBindings = static_cast<csize>(numNamespaces);
@@ -849,15 +852,7 @@
     nodeStack.push(NULL);
     pathStack.push(PathStepInfo(elemNode, baseUri));
   }
-  catch (ZorbaException const& e)
-  {
-    loader.theXQueryDiagnostics->add_error( e );
-  }
-  catch (...)
-  {
-    loader.theXQueryDiagnostics->
-    add_error(NEW_ZORBA_EXCEPTION(zerr::ZXQP0003_INTERNAL_ERROR));
-  }
+  LOADER_CATCH
 }
 
 
@@ -890,7 +885,7 @@
   XmlNode* prevChild = NULL;
   XmlNode* currChild;
 
-  try
+  LOADER_TRY
   {
     // Find the position of the 1st child of this element node in the node stack
     firstChildPos = stackSize - 1;
@@ -977,16 +972,7 @@
     }
 #endif
   }
-  catch (ZorbaException const& e)
-  {
-    loader.theXQueryDiagnostics->add_error( e );
-  }
-  catch (...)
-  {
-    loader.theXQueryDiagnostics->add_error(
-      NEW_ZORBA_EXCEPTION( zerr::ZXQP0003_INTERNAL_ERROR )
-    );
-  }
+  LOADER_CATCH
 }
 
 
@@ -1002,7 +988,7 @@
   FastXmlLoader& loader = *(static_cast<FastXmlLoader *>( ctx ));
   ZORBA_LOADER_CHECK_ERROR(loader);
 
-  try
+  LOADER_TRY
   {
     const char* charp = reinterpret_cast<const char*>(ch);
     zstring content(charp, len);
@@ -1028,16 +1014,7 @@
     LOADER_TRACE2(lSs.str());
 #endif
   }
-  catch (ZorbaException const& e)
-  {
-    loader.theXQueryDiagnostics->add_error( e );
-  }
-  catch (...)
-  {
-    loader.theXQueryDiagnostics->add_error(
-      NEW_ZORBA_EXCEPTION( zerr::ZXQP0003_INTERNAL_ERROR )
-    );
-  }
+  LOADER_CATCH
 }
 
 
@@ -1053,7 +1030,7 @@
   FastXmlLoader& loader = *(static_cast<FastXmlLoader *>( ctx ));
   ZORBA_LOADER_CHECK_ERROR(loader);
 
-  try
+  LOADER_TRY
   {
     // If a doc contains an element like <cdata><![CDATA[ <> ]]></cdata>,
     // libxml returns the string " <> ".
@@ -1081,16 +1058,7 @@
     LOADER_TRACE2(lSs.str());
 #endif
   }
-  catch (ZorbaException const& e)
-  {
-    loader.theXQueryDiagnostics->add_error( e );
-  }
-  catch (...)
-  {
-    loader.theXQueryDiagnostics->add_error(
-      NEW_ZORBA_EXCEPTION( zerr::ZXQP0003_INTERNAL_ERROR )
-    );
-  }
+  LOADER_CATCH
 }
 
 
@@ -1108,7 +1076,7 @@
   FastXmlLoader& loader = *(static_cast<FastXmlLoader *>( ctx ));
   ZORBA_LOADER_CHECK_ERROR(loader);
 
-  try
+  LOADER_TRY
   {
     // bugfix: handling PIs with no data (i.e. data being NULL)
     zstring content;
@@ -1131,16 +1099,7 @@
                   << targetp << std::endl << " ordpath = "
                   << piNode->getOrdPath().show() << std::endl);
   }
-  catch (ZorbaException const& e)
-  {
-    loader.theXQueryDiagnostics->add_error( e );
-  }
-  catch (...)
-  {
-    loader.theXQueryDiagnostics->add_error(
-      NEW_ZORBA_EXCEPTION( zerr::ZXQP0003_INTERNAL_ERROR )
-    );
-  }
+  LOADER_CATCH
 }
 
 
@@ -1155,7 +1114,7 @@
   FastXmlLoader& loader = *(static_cast<FastXmlLoader *>( ctx ));
   ZORBA_LOADER_CHECK_ERROR(loader);
 
-  try
+  LOADER_TRY
   {
     const char* charp = reinterpret_cast<const char*>(ch);
     zstring content;
@@ -1176,16 +1135,7 @@
                   << charp << std::endl << " ordpath = "
                   << commentNode->getOrdPath().show() << std::endl);
   }
-  catch (ZorbaException const& e)
-  {
-    loader.theXQueryDiagnostics->add_error( e );
-  }
-  catch (...)
-  {
-    loader.theXQueryDiagnostics->add_error(
-      NEW_ZORBA_EXCEPTION( zerr::ZXQP0003_INTERNAL_ERROR )
-    );
-  }
+  LOADER_CATCH
 }
 
 

=== modified file 'src/store/naive/node_items.cpp'
--- src/store/naive/node_items.cpp	2013-01-11 00:45:15 +0000
+++ src/store/naive/node_items.cpp	2013-02-03 18:00:35 +0000
@@ -751,7 +751,11 @@
       return (pos < numChildren);
     }
   }
-  catch(...)
+  catch (std::exception const &e)
+  {
+    ZORBA_FATAL(false, e.what());
+  }
+  catch (...)
   {
     ZORBA_FATAL(false, "Unexpected exception");
   }
@@ -825,6 +829,10 @@
 
     destroyInternal(removeType);
   }
+  catch (std::exception const &e)
+  {
+    ZORBA_FATAL(false, e.what());
+  }
   catch (...)
   {
     ZORBA_FATAL(false, "Unexpected exception");

=== modified file 'src/store/naive/node_updates.cpp'
--- src/store/naive/node_updates.cpp	2012-09-19 21:16:15 +0000
+++ src/store/naive/node_updates.cpp	2013-02-03 18:00:35 +0000
@@ -519,6 +519,10 @@
       SYNC_CODE(oldTree->getRCLock()->release());
     }
   }
+  catch (std::exception const &e)
+  {
+    ZORBA_FATAL(false, e.what());
+  }
   catch(...)
   {
     ZORBA_FATAL(0, "Unexpected exception");

=== modified file 'src/store/naive/pul_primitives.cpp'
--- src/store/naive/pul_primitives.cpp	2013-01-22 11:07:53 +0000
+++ src/store/naive/pul_primitives.cpp	2013-02-03 18:00:35 +0000
@@ -743,9 +743,13 @@
   {
     theRevalidationPul->applyUpdates(false);
   }
+  catch (std::exception const &e)
+  {
+    ZORBA_FATAL(false, "Error during the in-place validation: " << e.what());
+  }
   catch (...)
   {
-    ZORBA_FATAL(0, "Error during the in-place validation");
+    ZORBA_FATAL(false, "Error during the in-place validation");
   }
 
   theIsApplied = true;

=== modified file 'src/store/naive/store.cpp'
--- src/store/naive/store.cpp	2012-11-21 03:17:34 +0000
+++ src/store/naive/store.cpp	2013-02-03 18:00:35 +0000
@@ -574,9 +574,7 @@
   }
   catch(...)
   {
-    if (key != NULL)
-      delete key;
-
+    delete key;
     sourceIter->close();
     throw;
   }
@@ -1050,6 +1048,7 @@
   catch(...)
   {
     delete stream;
+    // catch-TODO: "throw;" here?
   }
   return docitem;
 }

=== modified file 'src/types/schema/schema.cpp'
--- src/types/schema/schema.cpp	2013-01-24 10:49:06 +0000
+++ src/types/schema/schema.cpp	2013-02-03 18:00:35 +0000
@@ -488,7 +488,7 @@
         ERROR_PARAMS( std::string(xsdURL), ZED( SchemaParseError ), StrX(e.getMessage())),
         ERROR_LOC( loc ) );
   }
-  catch (const ZorbaException& /* e */)
+  catch (const ZorbaException&)
   {
     throw;
   }
@@ -1763,18 +1763,6 @@
                  targetType->toSchemaString(),
                  msg));
   }
-  catch(const OutOfMemoryException&)
-  {
-    throw;
-  }
-  catch (const ZorbaException&) 
-  {
-    throw;
-  }
-  catch (...)
-  {
-    throw;
-  }
 #endif //ZORBA_NO_XMLSCHEMA
 
   // find the non user defined base type
@@ -2079,6 +2067,7 @@
        }
        catch (...)
        {
+        // catch-TODO: should all exceptions really be ignored here?
        }
 
        ar & binstr;