This is the formal review of Adam D. Ruppe's tool "dtoh" for inclusion in the tools repository [1].

Dtoh is a tool used to convert D modules to C/C++ headers. This allows to use D libraries in C/C++ code.

This review might be a bit special since this is the first time a tool is reviewed. Since this is a review of a tool the standard guidelines for reviewing might not apply. For example, we might require that the tool should have documentation like DMD does [3].

There's already a pull request with some discussion [2]. Note, the pull request has already been merged but we would like to do a review anyway.

Code: https://github.com/adamdruppe/tools/blob/dtoh/dtoh.d

The review will last for two weeks, ending on April 8.

[1] https://github.com/D-Programming-Language/tools
[2] https://github.com/D-Programming-Language/tools/pull/39
[3] http://dlang.org/dmd-osx.html

-- 
/Jacob Carlborg

On Tuesday, 25 March 2014 at 20:38:40 UTC, Jacob Carlborg wrote:
> This is the formal review of Adam D. Ruppe's tool "dtoh" for inclusion in the tools repository [1].
>
> Dtoh is a tool used to convert D modules to C/C++ headers. This allows to use D libraries in C/C++ code.
>
> This review might be a bit special since this is the first time a tool is reviewed. Since this is a review of a tool the standard guidelines for reviewing might not apply. For example, we might require that the tool should have documentation like DMD does [3].
>
> There's already a pull request with some discussion [2]. Note, the pull request has already been merged but we would like to do a review anyway.
>
> Code: https://github.com/adamdruppe/tools/blob/dtoh/dtoh.d
>
> The review will last for two weeks, ending on April 8.
>
> [1] https://github.com/D-Programming-Language/tools
> [2] https://github.com/D-Programming-Language/tools/pull/39
> [3] http://dlang.org/dmd-osx.html

At least it shouldn't give error if called without any params but give some info :)

Is there any usage example?

On 2014-03-26 16:33, Andrea Fontana wrote:

> At least it shouldn't give error if called without any params but give
> some info :)
>
> Is there any usage example?

Not that I have seen. Perhaps we should require some documentation.

-- 
/Jacob Carlborg

Right now it does not really look in shape for a formal review. Documentation is missing. Tool itself does not have help output and throws exception on plain "./dtoh" call. I see quite a lot of "FIXME" and "idea" comments in its source code. Missing internal DDOC comments at least for basic structures. In general it looks more like proof of concept than actual proposal.

Concept itself looks reasonable though. Eventually I'd like to see it use dmd frontend as a library but parsing JSON output should do fine until such option becomes feasible.

I don't really want to dwell deep into implementation right now because I think that before any further exploration can happen, tool must be made more friendly to collaborative work.

On Wednesday, 26 March 2014 at 15:33:47 UTC, Andrea Fontana wrote:
> Is there any usage example?

You make the json with dmd -X yourfile.d yourotherfiles.d

Then run the json through the thingy with ./dtoh yourfile.json

It'll make .h files for the extern(C) and extern(C++) pieces of the D files which should work to bring it into your other project.



What I really want it someone to try it in a vaguely real-world situation and see if it is helpful but could be better, too limited to be useful, or buggy or what. There's some things I think could be done better if dmd output more information, but I think this is somewhat useful as it is now and just needs some kind of serious test beyond my proof of concept file.

That's what I was hoping it could be pulled, with the note that it is super experimental, so maybe people will try to use it and file some bugs as to what really sucks. The whole approach might need to be abandoned in favor of a new dmd switch or something too. That's also why the style doesn't match and stuff like that; if it needs a rewrite anyway, no point getting worked up over brace placement. (It did need to be mostly rewritten a few months ago because dmd changed the json output!)

On Thursday, 27 March 2014 at 13:25:14 UTC, Adam D. Ruppe wrote:
> That's what I was hoping it could be pulled, with the note that it is super experimental, so maybe people will try to use it and file some bugs as to what really sucks. The whole approach might need to be abandoned in favor of a new dmd switch or something too. That's also why the style doesn't match and stuff like that; if it needs a rewrite anyway, no point getting worked up over brace placement. (It did need to be mostly rewritten a few months ago because dmd changed the json output!)

You still should provide better CLI, it does not rely on internal implementation and will make easier for random reviewers to test / experiment.

On Tuesday, 25 March 2014 at 20:38:40 UTC, Jacob Carlborg wrote:
> Code: https://github.com/adamdruppe/tools/blob/dtoh/dtoh.d

The author might consider using an associative array of functions to handle the various keywords, rather than switches.

I would suggest adding a "jsonutils.d" or somesuch, to break out the re-usable bits from the task-specific ones.

"scope (exit)" should be used to add the #endif to the end of the .h files. Also, there should be a comment with the guard name after it. (May as well try and create a nice-looking .h file).

On Thursday, 27 March 2014 at 18:28:28 UTC, Dicebot wrote:
> On Thursday, 27 March 2014 at 13:25:14 UTC, Adam D. Ruppe wrote:
>> That's what I was hoping it could be pulled, with the note that it is super experimental, so maybe people will try to use it and file some bugs as to what really sucks. The whole approach might need to be abandoned in favor of a new dmd switch or something too. That's also why the style doesn't match and stuff like that; if it needs a rewrite anyway, no point getting worked up over brace placement. (It did need to be mostly rewritten a few months ago because dmd changed the json output!)
>
> You still should provide better CLI, it does not rely on internal implementation and will make easier for random reviewers to test / experiment.

Want to extend this point a bit more : I believe that tools contributions should be evaluated based on a principle similar to Phobos modules - implementation may have flaws (or even changed completely) but user API (CLI + behavior for tools) should remain solid once merged. So it needs better attention even if you are going to re-write rest of the program later.

I'd consider it a blocker if this to undergo formal voting.

On 2014-03-27 12:27, Dicebot wrote:
> Right now it does not really look in shape for a formal review.
> Documentation is missing. Tool itself does not have help output and
> throws exception on plain "./dtoh" call. I see quite a lot of "FIXME"
> and "idea" comments in its source code. Missing internal DDOC comments
> at least for basic structures. In general it looks more like proof of
> concept than actual proposal.
>
> Concept itself looks reasonable though. Eventually I'd like to see it
> use dmd frontend as a library but parsing JSON output should do fine
> until such option becomes feasible.
>
> I don't really want to dwell deep into implementation right now because
> I think that before any further exploration can happen, tool must be
> made more friendly to collaborative work.

I fully agree with this.

-- 
/Jacob Carlborg

On 2014-03-25 21:38, Jacob Carlborg wrote:
> This is the formal review of Adam D. Ruppe's tool "dtoh" for inclusion
> in the tools repository [1].

The formal review of "dtoh" has now ended. I won't continue with voting since this the tool got very few reviews and in general doesn't feel ready.

-- 
/Jacob Carlborg