Skip to content

Commit

Permalink
Findings from review:
Browse files Browse the repository at this point in the history
1. Added a utils pkg with a Utils::rethrow() method
2. Defined a custom exception class: ExecutionGraph::OptionParserException
  • Loading branch information
tomuben committed Oct 10, 2024
1 parent 0d6d814 commit 43ecf04
Show file tree
Hide file tree
Showing 33 changed files with 110 additions and 66 deletions.
4 changes: 2 additions & 2 deletions exaudfclient/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ cc_binary(
name = "exaudfclient_bin",
srcs = ["exaudfclient.cc", "//base:load_dynamic"],
linkopts = ["-ldl"], # needed for dynamicly loading libexaudflib_complete.so into another linker namespace
deps = ["//base/exaudflib:header", "//base:debug_message_h"]+VM_ENABLED_DEPS+VM_PYTHON3_DEPS+
deps = ["//base/exaudflib:header", "//base/utils:utils"]+VM_ENABLED_DEPS+VM_PYTHON3_DEPS+
["//base/exaudflib:exaudflib-deps", "//base/swig_factory:swig_factory"],
defines = VM_ENABLED_DEFINES,
data = ["//base:libexaudflib_complete.so"]
Expand All @@ -100,7 +100,7 @@ cc_binary(
name = "exaudfclient_static_bin",
srcs = ["exaudfclient.cc", "//base:load_dynamic"],
linkopts = ["-ldl"], # needed for dynamicly loading libexaudflib_complete.so into another linker namespace
deps = ["//base/exaudflib:header", "//base:debug_message_h"]+VM_ENABLED_DEPS+VM_PYTHON3_DEPS+
deps = ["//base/exaudflib:header", "//base/utils:utils"]+VM_ENABLED_DEPS+VM_PYTHON3_DEPS+
["//base/exaudflib:exaudflib-deps", "//base/swig_factory:swig_factory"] +
[ "@zmq//:zmq", "@protobuf//:protobuf"],
defines = VM_ENABLED_DEFINES,
Expand Down
8 changes: 1 addition & 7 deletions exaudfclient/base/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,13 @@ exports_files(["filter_swig_code.py", "build_integrated.py",

load("//:variables.bzl", "VM_ENABLED_DEFINES")

cc_library(
name = "debug_message_h",
hdrs = [
"debug_message.h"
],
)

cc_library(
name = "load_dynamic",
srcs = [
"load_dynamic.cc"
],
deps = ["//base/exaudflib:header", "//base:debug_message_h", "//base/exaudflib:exaudflib-deps"],
deps = ["//base/exaudflib:header", "//base/utils:utils", "//base/exaudflib:exaudflib-deps"],
defines = VM_ENABLED_DEFINES,
)

Expand Down
2 changes: 1 addition & 1 deletion exaudfclient/base/benchmark_container/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ cc_library(
name = "benchmark_container",
srcs = ["benchmark_container.cc","benchmark_container.h"],
hdrs = ["benchmark_container.h"],
deps = ["//base/exaudflib:exaudflib-deps","//base/exaudflib:header", "//base:debug_message_h"]+["//base/script_options_parser:scriptoptionlines"]
deps = ["//base/exaudflib:exaudflib-deps","//base/exaudflib:header", "//base/utils:utils"]+["//base/script_options_parser:scriptoptionlines"]
)
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#define ENABLE_BENCHMARK_VM
#endif

#include "base/debug_message.h"
#include "base/utils/debug_message.h"
#include "benchmark_container.h"
#include <iostream>
#include <string>
Expand Down
2 changes: 1 addition & 1 deletion exaudfclient/base/exaudflib/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -94,5 +94,5 @@ cc_library(
"impl/swig/swig_general_iterator.h", "impl/swig/swig_general_iterator.cc"],
defines = VM_ENABLED_DEFINES,
linkstatic = False, # Needs to be false, because otherwise we might get missing symbols when loading exaudflib_complete.so
deps = [":exaudflib-deps", ":zmqcontainer", "@zmq//:zmq", ":header", "//base:debug_message_h"],
deps = [":exaudflib-deps", ":zmqcontainer", "@zmq//:zmq", ":header", "//base/utils:utils"],
)
2 changes: 1 addition & 1 deletion exaudfclient/base/exaudflib/impl/check.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#include <atomic>


#include "base/debug_message.h"
#include "base/utils/debug_message.h"

namespace exaudflib {
namespace check {
Expand Down
2 changes: 1 addition & 1 deletion exaudfclient/base/exaudflib/impl/exaudflib_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include <fstream>
#include <functional>

#include "base/debug_message.h"
#include "base/utils/debug_message.h"

// swig lib
#include <limits>
Expand Down
2 changes: 1 addition & 1 deletion exaudfclient/base/exaudflib/impl/socket_high_level.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#include "base/exaudflib/swig/swig_common.h"
#include "base/exaudflib/vm/swig_vm.h"
#include <sys/resource.h>
#include "base/debug_message.h"
#include "base/utils/debug_message.h"

namespace exaudflib {
namespace socket_high_level {
Expand Down
2 changes: 1 addition & 1 deletion exaudfclient/base/exaudflib/impl/socket_low_level.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#include "base/exaudflib/impl/socket_low_level.h"
#include "base/exaudflib/impl/check.h"
#include "base/debug_message.h"
#include "base/utils/debug_message.h"
#include <mutex>
#include <iostream>
#include <unistd.h>
Expand Down
2 changes: 1 addition & 1 deletion exaudfclient/base/javacontainer/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ cc_library(
srcs = [":javacontainer.cc", ":javacontainer.h", ":javacontainer_impl.cc", ":javacontainer_impl.h", ":dummy"],
hdrs = [":filter_swig_code_exascript_java_h", "exascript_java_jni_decl.h"],
deps = ["@ssl//:ssl","@java//:java", ":exascript_java", "//base/exaudflib:header",
"//base:debug_message_h","//base/javacontainer/script_options:java_script_option_lines",
"//base/utils:utils","//base/javacontainer/script_options:java_script_option_lines",
"//base/swig_factory:swig_factory_if"],
# copts= ["-O0","-fno-lto"],
alwayslink=True,
Expand Down
21 changes: 9 additions & 12 deletions exaudfclient/base/javacontainer/javacontainer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

#include "exascript_java_jni_decl.h"

#include "base/debug_message.h"
#include "base/utils/debug_message.h"
#include "base/javacontainer/javacontainer.h"
#include "base/javacontainer/javacontainer_impl.h"
#include "base/javacontainer/script_options/extractor.h"
Expand All @@ -35,21 +35,18 @@ JavaVMImpl::JavaVMImpl(bool checkOnly, bool noJNI, SwigFactory& swigFactory)
m_exaJavaPath = "/exaudf/base/javacontainer"; // TODO hardcoded path

JavaScriptOptions::ScriptOptionLinesParserLegacy scriptOptionsParser;
try {
JavaScriptOptions::Extractor extractor(scriptOptionsParser, swigFactory);

DBG_FUNC_CALL(cerr,extractor.extract(m_scriptCode)); // To be called before scripts are imported. Otherwise, the script classname from an imported script could be used
JavaScriptOptions::Extractor extractor(scriptOptionsParser, swigFactory);

DBG_FUNC_CALL(cerr,setClasspath());
DBG_FUNC_CALL(cerr,extractor.extract(m_scriptCode)); // To be called before scripts are imported. Otherwise, the script classname from an imported script could be used

m_jvmOptions = std::move(extractor.moveJvmOptions());
DBG_FUNC_CALL(cerr,setClasspath());

for (set<string>::iterator it = extractor.getJarPaths().begin(); it != extractor.getJarPaths().end();
++it) {
addJarToClasspath(*it);
}
} catch(const std::runtime_error& ex) {
throwException(ex);
m_jvmOptions = std::move(extractor.moveJvmOptions());

for (set<string>::iterator it = extractor.getJarPaths().begin(); it != extractor.getJarPaths().end();
++it) {
addJarToClasspath(*it);
}

m_needsCompilation = checkNeedsCompilation();
Expand Down
5 changes: 3 additions & 2 deletions exaudfclient/base/javacontainer/script_options/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ cc_library(
hdrs = [":extractor.h", ":parser_legacy.h"],
srcs = [":parser.h", ":converter.h", ":converter.cc", ":parser_legacy.cc", ":extractor.cc",
":keywords.h", ":checksum.h", ":checksum.cc"],
deps = ["//base/script_options_parser/legacy:script_option_lines_parser_legacy", "//base:debug_message_h",
"//base/exaudflib:header", "//base/exaudflib:exaudflib-deps", "//base/swig_factory:swig_factory_if"],
deps = ["//base/script_options_parser/legacy:script_option_lines_parser_legacy", "//base/utils:utils",
"//base/exaudflib:header", "//base/exaudflib:exaudflib-deps", "//base/swig_factory:swig_factory_if",
"//base/script_options_parser:exception"],
)
1 change: 0 additions & 1 deletion exaudfclient/base/javacontainer/script_options/converter.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

#include <string>
#include <vector>
#include <functional>
#include <set>
#include <memory>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#include "base/javacontainer/script_options/extractor.h"
#include "base/javacontainer/script_options/parser.h"

#include "base/debug_message.h"
#include "base/utils/debug_message.h"
#include <iostream>

#define EXTR_DBG_FUNC_CALL(f) DBG_FUNC_CALL(std::cerr, f)
Expand Down
1 change: 0 additions & 1 deletion exaudfclient/base/javacontainer/script_options/extractor.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

#include <string>
#include <vector>
#include <functional>
#include <set>


Expand Down
28 changes: 14 additions & 14 deletions exaudfclient/base/javacontainer/script_options/parser_legacy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
#include "base/script_options_parser/legacy/script_option_lines.h"
#include "base/exaudflib/swig/swig_meta_data.h"
#include "base/swig_factory/swig_factory.h"
#include "base/utils/exceptions.h"
#include "base/script_options_parser/exception.h"

#include <memory>
#include <stdexcept>



namespace SWIGVMContainers {
Expand Down Expand Up @@ -106,8 +106,8 @@ void ScriptOptionLinesParserLegacy::extractImportScripts(SwigFactory & swigFacto
try {
parseForSingleOption(m_keywords.importKeyword(),
[&](const std::string& value, size_t pos){scriptPos = pos; newScript = value;});
} catch (const std::runtime_error & ex) {
throw std::runtime_error(std::string("F-UDF-CL-SL-JAVA-1614 ") + ex.what());
} catch (const ExecutionGraph::OptionParserException & ex) {
Utils::rethrow(ex, "F-UDF-CL-SL-JAVA-1614");
}
if (!newScript.empty()) {
if (!metaData) {
Expand Down Expand Up @@ -135,26 +135,26 @@ void ScriptOptionLinesParserLegacy::parseForScriptClass(std::function<void(const
try {
parseForSingleOption(m_keywords.scriptClassKeyword(),
[&](const std::string& value, size_t pos){callback(value);});
} catch (const std::runtime_error& ex) {
throw std::runtime_error(std::string("F-UDF-CL-SL-JAVA-1610") + ex.what());
} catch (const ExecutionGraph::OptionParserException& ex) {
Utils::rethrow(ex, "F-UDF-CL-SL-JAVA-1610");
}
}

void ScriptOptionLinesParserLegacy::parseForJvmOptions(std::function<void(const std::string &option)> callback) {
try {
parseForMultipleOptions(m_keywords.jvmOptionKeyword(),
[&](const std::string& value, size_t pos){callback(value);});
} catch(const std::runtime_error& ex) {
throw std::runtime_error(std::string("F-UDF-CL-SL-JAVA-1612") + ex.what());
} catch(const ExecutionGraph::OptionParserException& ex) {
Utils::rethrow(ex, "F-UDF-CL-SL-JAVA-1612");
}
}

void ScriptOptionLinesParserLegacy::parseForExternalJars(std::function<void(const std::string &option)> callback) {
try {
parseForMultipleOptions(m_keywords.jarKeyword(),
[&](const std::string& value, size_t pos){callback(value);});
} catch(const std::runtime_error& ex) {
throw std::runtime_error(std::string("F-UDF-CL-SL-JAVA-1613") + ex.what());
} catch(const ExecutionGraph::OptionParserException& ex) {
Utils::rethrow(ex, "F-UDF-CL-SL-JAVA-1613");
}
}

Expand All @@ -171,8 +171,8 @@ void ScriptOptionLinesParserLegacy::parseForSingleOption(const std::string keywo
if (option != "") {
callback(option, pos);
}
} catch(const std::runtime_error& ex) {
throw std::runtime_error(std::string("F-UDF-CL-SL-JAVA-1606") + ex.what());
} catch(const ExecutionGraph::OptionParserException& ex) {
Utils::rethrow(ex, "F-UDF-CL-SL-JAVA-1606");
}
}

Expand All @@ -186,8 +186,8 @@ void ScriptOptionLinesParserLegacy::parseForMultipleOptions(const std::string ke
if (option == "")
break;
callback(option, pos);
} catch(const std::runtime_error& ex) {
throw std::runtime_error(std::string("F-UDF-CL-SL-JAVA-1607") + ex.what());
} catch(const ExecutionGraph::OptionParserException& ex) {
Utils::rethrow(ex, "F-UDF-CL-SL-JAVA-1607");
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions exaudfclient/base/python/python3/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ cc_library(
hdrs = [":exascript_python_int", ":filter_swig_code_exascript_python_h"],
include_prefix = ".",
deps = ["@python3//:python3",":exascript_python","//base/exaudflib:header",
"//base:debug_message_h", "//base/python:pythoncontainer_header"],
"//base/utils:utils", "//base/python:pythoncontainer_header"],
alwayslink=True,
)

Expand All @@ -109,5 +109,5 @@ cc_binary(
linkshared = 1,
srcs = [":python_ext_dataframe.cc"],
deps = ["@python3//:python3","@numpy//:numpy",
"//base/exaudflib:header", "//base:debug_message_h"],
"//base/exaudflib:header", "//base/utils:utils"],
)
2 changes: 1 addition & 1 deletion exaudfclient/base/python/python3/python_ext_dataframe.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#include <algorithm>
#include "base/exaudflib/swig/swig_common.h"
#include "base/debug_message.h"
#include "base/utils/debug_message.h"

#include <Python.h>

Expand Down
2 changes: 1 addition & 1 deletion exaudfclient/base/python/pythoncontainer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#include <Python.h>
#include "exascript_python_int.h"
#include "exascript_python.h"
#include "base/debug_message.h"
#include "base/utils/debug_message.h"

#include "base/exaudflib/swig/script_data_transfer_objects.h"

Expand Down
5 changes: 5 additions & 0 deletions exaudfclient/base/script_options_parser/BUILD
Original file line number Diff line number Diff line change
@@ -1 +1,6 @@
package(default_visibility = ["//visibility:public"])

cc_library(
name = "exception",
hdrs = ["exception.h"]
)
1 change: 1 addition & 0 deletions exaudfclient/base/script_options_parser/ctpg/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ cc_library(
name = "script_option_lines_parser_ctpg",
hdrs = ["script_option_lines_ctpg.h"],
srcs = ["script_option_lines_ctpg.cc","ctpg.hpp"],
deps = ["//base/script_options_parser:exception"],
copts= ["-fno-lto"],
)
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <iostream>
#include <string>
#include <sstream>
#include "base/script_options_parser/exception.h"

using namespace exaudf_ctpg;
using namespace exaudf_ctpg::ftors;
Expand Down Expand Up @@ -147,7 +148,7 @@ void parse(std::string&& code, options_type& result) {
{
std::stringstream ss;
ss << "Error parsing script options: " << error_buffer.str();
throw std::runtime_error(ss.str());
throw OptionParserException(ss.str());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
#define SCRIPTOPTIONLINESCTPG_H

#include <string>
#include <functional>
#include <vector>
#include <map>
#include <ostream>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
#include <gmock/gmock.h>
#include <string>
#include <exception>
#include "base/script_options_parser/exception.h"


using namespace ExecutionGraph;
using namespace ExecutionGraph::OptionsLineParser::CTPG;

using ::testing::MatchesRegex;
Expand Down Expand Up @@ -75,13 +77,13 @@ TEST(ScriptOptionLinesTest, need_option_termination_character) {
{
parseOptions(code, result);
}
catch( const std::runtime_error& e )
catch( const OptionParserException& e )
{
// and this tests that it has the correct message
EXPECT_STREQ( e.what(), "Error parsing script options: [1:17] PARSE: Syntax error: Unexpected '<eof>'\n");
throw;
}
}, std::runtime_error );
}, OptionParserException );
}

TEST(ScriptOptionLinesTest, finds_the_two_options_same_key) {
Expand Down Expand Up @@ -131,12 +133,12 @@ TEST_P(ScriptOptionLinesInvalidOptionTest, value_is_mandatory) {
{
parseOptions(code, result);
}
catch( const std::runtime_error& e )
catch( const OptionParserException& e )
{
EXPECT_THAT( e.what(), MatchesRegex("^Error parsing script options.*PARSE: Syntax error: Unexpected.*$"));
throw;
}
}, std::runtime_error );
}, OptionParserException );
}

const std::vector<std::string> invalid_options = {"%some_option ;", "%some_option \n", "\n%some_option\n;", "%some_option\nvalue;"};
Expand Down
17 changes: 17 additions & 0 deletions exaudfclient/base/script_options_parser/exception.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#ifndef SCRIPTOPTIONLINESEXCEPTION_H
#define SCRIPTOPTIONLINESEXCEPTION_H

#include <stdexcept>


namespace ExecutionGraph
{

class OptionParserException : public std::runtime_error {
using std::runtime_error::runtime_error;
};


} // namespace ExecutionGraph

#endif // SCRIPTOPTIONLINESEXCEPTION_H
1 change: 1 addition & 0 deletions exaudfclient/base/script_options_parser/legacy/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ cc_library(
name = "script_option_lines_parser_legacy",
hdrs = ["script_option_lines.h"],
srcs = ["script_option_lines.cc"],
deps = ["//base/script_options_parser:exception"],
copts= ["-fno-lto"],
)
Loading

0 comments on commit 43ecf04

Please sign in to comment.