Wip/cpp #4

vendethiel · 2018-07-20T13:45:14Z

No description provided.

ingydotnet

Good work so far. Mostly just have suggestions for keeping this codebase more in sync with the others.

ingydotnet · 2018-07-20T13:53:58Z

src/cpp/lib/testml/runtime.cpp

+    _data = _ast["data"];
+  }
+
+  json Runtime::exec_expr(json fragment) {


The variable 'fragment' should be called 'expr' to match all the other implementations. Let's try to use the same names as much as possible across code bases. If a name conflicts for some reason I typically add an '_' to the end or some other predetermined unique-ifying method.

It's important for maintainers to be able to look at all the code bases and expect similar names, style and layout.

There will be times when people need to add a feature across 20 langs at once, and it should be relatively straightforward.

ingydotnet · 2018-07-20T14:01:05Z

src/cpp/lib/testml/runtime.cpp

+
+  json Runtime::exec_dot(json::array_t fragment) {
+    json context = {}; // result of last call
+


This line has trailing whitespace. I have a vim plugin that highlights it in red :)

Maybe we can add some kind of git check hook to prevent things like this being committed/pushed.

ingydotnet · 2018-07-20T14:07:22Z

src/cpp/lib/testml/runtime.cpp

+namespace testml {
+
+  namespace {
+    bool is_all_lowercase(std::string const& s) {


Functions like this should go into a testml/util.cpp.

ingydotnet · 2018-07-20T14:10:20Z

src/cpp/lib/testml/runtime.cpp

+    return _currentBlock["point"][name];
+  }
+
+  json Runtime::exec_dot(json::array_t fragment) {


fragment should be 'calls'

ingydotnet · 2018-07-20T14:13:45Z

src/cpp/lib/testml/runtime.cpp

+    return value[0].is_string() && value[0] == "=>";
+  }
+
+  void Runtime::run() {


This function should be called 'test'. Also try to put the methods in the same order as the coffee and others. All the others are in sync in this way.

ingydotnet · 2018-07-20T14:15:38Z

src/cpp/lib/testml/runtime.cpp

+    for (auto& statement : _ast["code"]) {
+      exec_expr(statement);
+    }
+    testml_done();


This method should be 'testml_end'. I know it's just naming, but let's start out keeping things in sync. Makes it easier for me and others to jump in and help.

ingydotnet · 2018-07-20T14:17:47Z

src/cpp/lib/testml/wrapper.cpp

@@ -0,0 +1,29 @@
+#include "wrapper.hpp"


Why can't the wrapper methods go inside the runtime class? ie Why do we need a separate class here?

It's not a separate class. These functions will supposedly be reused by the Bridge"-r" and the Stdlib class, that's why it's separated into its own file.

I'm not certain that the bridge classes will (or should) know about and use testml runtime internals, but we can wait and see.

I don't understand.

@vendethiel now that we forged a common understanding on IRC, I get where you are going.

In TestML lingo a 'method' call like .foo or .Bar is called a 'callable'. .foo is a user defined bridge class callable (because it is lower case) and .Bar is a stdlib callable. Given that the testml::Bridge (base class for user defined TestMLBridge class) and testml::StdLib both need the wrapper stuff I think that wrapper should be a testml::Callable class and both testml::Bridge and testml::StdLib should inherit from it.

ingydotnet · 2018-07-20T14:19:58Z

src/cpp/src/testml-run.cpp

-  TestML_Run_TAP::run(file);
-//   tap.run();
+  testml::Bridge bridge;
+  bridge.bind("add", +[](int a, int b) {


These functions (supposedly user defined) need to be in test/testml-bridge.cpp. That's where a user will add the C++ methods need to make the tests run.

ingydotnet · 2018-07-20T14:25:26Z

src/cpp/lib/testml/bridge.cpp

+
+  using json = nlohmann::json;
+
+  json Bridge::call(std::string const& name, std::vector<json> const& args) {


I don't think the call method belongs in the bridge base class. This implies that a user defined bridge method would want to call this method directly. Maybe this method should be called can and it returns the bridge method pointer of the requested method, if found.

ingydotnet · 2018-07-21T18:44:31Z

@vendethiel can you git rebase master && make clean test && git push -f on your wip/cpp branch? That will fix make test running on a fresh clone...

done

ingydotnet · 2018-08-12T17:30:59Z

@vendethiel you should join #testml again. I'm in a nearby TZ for a while...

Needed to run: make `test-runtime-cpp` from root dir.

ingydotnet · 2018-11-11T18:04:35Z

C++ might be better implemented by generating C++ code from the Lingy AST.

That's how I did Bash support for testml. Walking a JSON structure seemed like
it was going to be tricky. Generating Bash code was much simpler, though I
haven't passed all the test suite yet.

@vendethiel been looking for you on #testml

Would like to discuss this approach with you.

vendethiel · 2019-03-21T13:15:41Z

I've been connected a few times a week, rebased the PR late 2018.

ingydotnet previously requested changes Jul 20, 2018

View reviewed changes

vendethiel force-pushed the wip/cpp branch 2 times, most recently from b47ad45 to e568fdd Compare July 23, 2018 13:45

ingydotnet force-pushed the wip/cpp branch 3 times, most recently from bc2d9c7 to 870b2a2 Compare July 25, 2018 17:43

vendethiel force-pushed the wip/cpp branch 2 times, most recently from e3477a2 to 870b2a2 Compare October 17, 2018 15:50

vendethiel and others added 11 commits October 17, 2018 17:50

WIP C++ impl

bc6a1fa

Start working on the Bridge class

8385002

Implement bridges

a418514

add wrapper class

5f7ff6c

Makefile rule to fetch nlohmann/json.hpp

09014be

Hackery hackety hacks for PMFs

27979da

small adjustements

7bc87c1

add boost::callable_traits

7cae02f

Add json.hpp directly to the repository (for now)

d2e5956

Add C++ bin symlinks

6201219

Needed to run: make `test-runtime-cpp` from root dir.

Replace local ext/ with symlink to ext/cpp branch

08f2b6f

vendethiel force-pushed the wip/cpp branch from 870b2a2 to 08f2b6f Compare October 17, 2018 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wip/cpp #4

Wip/cpp #4

vendethiel commented Jul 20, 2018

ingydotnet left a comment

ingydotnet Jul 20, 2018

ingydotnet Jul 20, 2018

ingydotnet Jul 20, 2018

ingydotnet Jul 20, 2018

ingydotnet Jul 20, 2018

ingydotnet Jul 20, 2018

ingydotnet Jul 20, 2018

vendethiel Jul 21, 2018

ingydotnet Jul 21, 2018

vendethiel Jul 21, 2018

ingydotnet Jul 21, 2018

ingydotnet Jul 20, 2018

ingydotnet Jul 20, 2018

ingydotnet commented Jul 21, 2018

ingydotnet commented Aug 12, 2018

ingydotnet commented Nov 11, 2018

vendethiel commented Mar 21, 2019


		json Runtime::exec_dot(json::array_t fragment) {
		json context = {}; // result of last call


		using json = nlohmann::json;

		json Bridge::call(std::string const& name, std::vector<json> const& args) {

Wip/cpp #4

Are you sure you want to change the base?

Wip/cpp #4

Conversation

vendethiel commented Jul 20, 2018

ingydotnet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingydotnet commented Jul 21, 2018

ingydotnet commented Aug 12, 2018

ingydotnet commented Nov 11, 2018

vendethiel commented Mar 21, 2019