Convert the JSON code to use D slices instead of pointers by Geod24 · Pull Request #8555 · dlang/dmd

Geod24 · 2018-08-10T04:47:36Z

Since we want to get rid of strlen, I tried to convert a simple global to a D slice and was blocked by the JSON module. So here is the module conversion to D.
Some simple things were added (e.g. RootObject.toString which just strlen on toChars), but I avoided changing the AST as much as possible, so things like kind() and Loc.filename can be done in another PR.

dlang-bot · 2018-08-10T04:47:37Z

Thanks for your pull request and interest in making D better, @Geod24! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8555"

WalterBright · 2018-08-10T07:07:37Z

src/dmd/json.d

        if (type)
        {
-            property(name, type.toChars());
+            property(name, RootObject.toString(type));


Not too thrilled about this. It should be type.toString(). Yes, the C++ code cannot call it, but all you need to do is put a dummy function declaration there on the C++ side so the vtbl[] entries line up.

JinShil · 2018-08-10T07:59:42Z

src/dmd/dmodule.d

        return buf.extractString();
    }
+
+    extern (D) const(char)[] toString() const


As I mentioned before, toString() is used pervasively in dmd, and as long as it does what the other toString()'s do, adding a Ddoc for it is redundant.

While I agree that a comment is not really necessary here, you have to add some ddoc comment to make the function appear in the documentation at all. Not sure if that is a good thing as you have to litter the code with empty comments this way. Is there an option to put all (public) symbols into the documentation?

For the record I added a (trivial) DDOC comment just for this reason (documentation)

JinShil · 2018-08-10T07:59:53Z

src/dmd/hdrgen.d

    return null;
 }

+extern (D) const(char)[] stcToString(ref StorageClass stc)


JinShil · 2018-08-10T08:00:21Z

src/dmd/hdrgen.d

+    return trustToString(trust).ptr;
+}
+
+extern (D) string trustToString(TRUST trust)


JinShil · 2018-08-10T08:00:29Z

src/dmd/hdrgen.d

+    return protectionToString(kind).ptr;
+}
+
+extern (D) string protectionToString(Prot.Kind kind)


jacob-carlborg · 2018-08-10T08:06:58Z

src/dmd/json.d

+            if (auto filename = toDString(loc.filename))
            {
-                if (!this.filename || strcmp(filename, this.filename))
+                if (!this.filename.length || filename != this.filename)


I'm guessing you're trying keep the new to code be as close as possible to the old code but this first expression (!this.filename.length) isn't needed anymore?

Geod24 · 2018-08-13T01:44:44Z

Updated according to feedback

ibuclaw · 2018-08-13T05:37:33Z

src/dmd/dmodule.d

        return buf.extractString();
    }
+
+    /// Provide an human readable representation


Provide a human

ibuclaw · 2018-08-13T05:37:53Z

src/dmd/hdrgen.d

+
+/**
+ * Returns:
+ *   an human readable representation of `stc`


ibuclaw · 2018-08-13T05:38:57Z

src/dmd/hdrgen.d


+/**
+ * Returns:
+ *   an human readable representation of `trust`,


ibuclaw · 2018-08-13T05:41:34Z

src/dmd/hdrgen.d


+/**
+ * Returns:
+ *   an human readable representation of `kind`


ibuclaw · 2018-08-13T05:46:00Z

src/dmd/json.d

    }

-    void stringPart(const(char)* s)
+    extern(D) void stringPart(const(char)[] s)


Wouldn't all non-visit members of this visitor be private? Ergo you can just add extern(D): at the top of this body rather than adding it for each individual function.

Think about it for future refactor.

True that. At the moment I'm putting extern(D) right and left (got more slice-ification coming), but I also had weird bugs (compiler SEGV) being triggered in the past some I decided to go for verbose and simple. But when enough is converted, we surely can do that.

ibuclaw · 2018-08-13T05:52:37Z

src/dmd/root/outbuffer.d

    } while (value);
    return buf[i .. $];
 }
-


Seems like a leftover from a previous attempt here.

It is, but also its own commit

Geod24 · 2018-08-13T06:45:26Z

Fixed ~~typos~~ poor English

wilzbach · 2018-08-13T15:53:50Z

src/dmd/json.d

+    static extern(D) const(char)[] toDString (const(char)* s)
+    {
+        return s ? s[0 .. strlen(s)] : null;
+    }


Why not put this as a global function into utils or similar? It's used at other places too and doesn't really fit in ToJsonVisitor

Didn't expect it to be so widely used :(
Also I'm wary that dmd.utils will be imported everywhere soon.
But let's give it a shot...

wilzbach · 2018-08-13T15:54:42Z

src/dmd/hdrgen.d

+extern (D) const(char)[] stcToString(ref StorageClass stc)
+{
+    auto p = stcToChars(stc);
+    return p[0 .. strlen(p)];


Could use toDString from below.

wilzbach · 2018-08-13T15:54:56Z

src/dmd/dmodule.d

+    extern (D) const(char)[] toString() const
+    {
+        auto p = this.toChars();
+        return p[0 .. strlen(p)];


Could use toDString from below.

wilzbach · 2018-08-13T15:56:27Z

src/dmd/json.d

-        property("kind", s.kind());
-        property("comment", s.comment);
+        property("kind", toDString(s.kind()));
+        property("comment", toDString(s.comment));


Nit: another advantage of making toDString a global function is that you can do the more natural: s.comment.toDString()

wilzbach · 2018-08-13T15:57:42Z

src/dmd/root/rootobject.d

+    {
+        import core.stdc.string : strlen;
+        auto p = this.toChars();
+        return p[0 .. strlen(p)];


Could use toDString from above.

Here I choose not to, as we want rootobject to depends on as little as possible.

Geod24 · 2018-08-13T16:32:17Z

Addressed @wilzbach 's comments

JinShil

Still looking good.

Geod24 · 2018-08-14T05:38:11Z

Since this has gotten a bit more reviews, and one might be wondering where I am going, I added a bit more diff than originally intended:

Commit 0c91769 moves DArray from globals.h to a standalone header, because we don't want object.h importing globals.h
~~I reworked RootObject.toString to be callable from C++ and added a test.~~
Only works on POSIX x64 :'(

As for the end goal is, it's to be able to do this gradually.

jacob-carlborg · 2018-08-14T05:51:38Z

src/tests/cxxfrontend.c

    semantic3(m, NULL);

+    // Basic test for D slice passing
+    DArray<const char> sl = m->toString();


Does this work on Windows? I’ve been told D arrays are returned in registers while structs of this size are not on Windows.

I should've known it was too good to be true.
I'll wait for the auto-tester to tell me what's the situation with Windows, but it fails on 32 bits.
So I guess I will take that test out for now :'(

Hum, the C++ frontend tests only run on POSIX anyway...

For reference: #8120 (comment).

jacob-carlborg · 2018-08-14T05:53:22Z

src/dmd/utils.d

 }
+
+/// Slices a `\0`-terminated C-string, excluding the terminator
+const(char)[] toDString (const(char)* s) pure nothrow @nogc


Why not toString?

toString is to provide a string representation, this is to convert a const(char)* to a const(char)[].

Geod24 · 2018-08-14T07:40:02Z

@wilzbach :

dmd/root/rootobject.d(66:29)[warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.

What's the deal with that warning ?? extern(C++) classes don't inherit from Object so it doesn't make sense to require them to be const. And I currently cannot make it const.

wilzbach · 2018-08-14T08:56:47Z

What's the deal with that warning ?? extern(C++) classes don't inherit from Object so it doesn't make sense to require them to be const.

Yeah that's the newly added Dscanner static code analysis check and the idea is that the checks slowly allow to modernize DMD / enforce some automatic code checking.
I guess the assumption behind this check was that "opEquals, opCmp, toHash, and toString are either const, immutable or inout" because they shouldn't be changing any data.

And I currently cannot make it const.

tl;dr: in this case, add your module to this list:

dmd/src/.dscanner.ini

Line 118 in 48726c8

object_const_check="-dmd.dtemplate,-dmd.backend.outbuf"

Geod24 · 2018-08-14T09:06:30Z

@wilzbach : Thanks, glad to see there's a way out of it!

jacob-carlborg · 2018-08-14T09:04:13Z

src/dmd/root/object.h

    virtual const char *toChars();
+    /// This function is `extern(D)` and should not be called from C++,
+    /// as the ABI does not match on some platforms
+    virtual DArray<const char> toString();


Is it possible to implement this on the C++ and have the implementation assert?

I don't think so.

wilzbach · 2018-08-14T11:27:47Z

@Geod24 I don't know exactly what's up with SemaphoreCI, but #8560 (once merged back to master) will hopefully help a bit.

If we want to use DArray from dmd.root it cannot reside in globals.h, so it is moved to a standalone header.

At the moment it is simply doing an `strlen` on the result of `toChars`, but as more code uses it, we can start to move away from that. Another reason to do it this way is because code can rely on the string being null terminated, so this way `x.toString().ptr` is still a drop-in replacement of `x.toChars()`.

strlen must die, right ?

The conversion is pretty straightforward, but required in order to convert globals. Since globals are still C-style string, a local toDString is used. It can later be removed as globals are converted to D slices.

Geod24 · 2018-08-14T16:00:57Z

Rebased on master, let's see.

JinShil

Yup, I still like it.

wilzbach

Yeah same here. One small step forward (:

Geod24 · 2018-08-16T04:31:31Z

Anything else ? Got a few changes depending on this PR (to be precise, on toDString).

wilzbach · 2018-08-16T08:35:42Z

I think this is just pending/blocked on Ian's change request. @ibuclaw are you okay with moving forward here?

Geod24 requested a review from WalterBright as a code owner August 10, 2018 04:47

Geod24 force-pushed the json branch from ce14536 to 1dad8a3 Compare August 10, 2018 05:04

Geod24 requested review from dnadlinger and ibuclaw as code owners August 10, 2018 05:04

Geod24 force-pushed the json branch 3 times, most recently from 7934d97 to 310aa16 Compare August 10, 2018 06:39

WalterBright reviewed Aug 10, 2018

View reviewed changes

Geod24 force-pushed the json branch from 310aa16 to 6e89d6c Compare August 10, 2018 07:30

JinShil reviewed Aug 10, 2018

View reviewed changes

src/dmd/hdrgen.d

return null;

}

extern (D) const(char)[] stcToString(ref StorageClass stc)

Copy link

Contributor

JinShil Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DDoc?

JinShil reviewed Aug 10, 2018

View reviewed changes

src/dmd/hdrgen.d

return trustToString(trust).ptr;

}

extern (D) string trustToString(TRUST trust)

Copy link

Contributor

JinShil Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DDoc?

JinShil reviewed Aug 10, 2018

View reviewed changes

src/dmd/hdrgen.d

return protectionToString(kind).ptr;

}

extern (D) string protectionToString(Prot.Kind kind)

Copy link

Contributor

JinShil Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DDoc?

jacob-carlborg reviewed Aug 10, 2018

View reviewed changes

jacob-carlborg approved these changes Aug 10, 2018

View reviewed changes

Geod24 force-pushed the json branch from 6e89d6c to a5d8a19 Compare August 13, 2018 01:44

JinShil approved these changes Aug 13, 2018

View reviewed changes

ibuclaw requested changes Aug 13, 2018

View reviewed changes

Geod24 force-pushed the json branch from a5d8a19 to 130d696 Compare August 13, 2018 06:45

wilzbach reviewed Aug 13, 2018

View reviewed changes

Geod24 force-pushed the json branch from 130d696 to d57949e Compare August 13, 2018 16:31

Geod24 force-pushed the json branch from d57949e to 162b043 Compare August 14, 2018 00:16

JinShil approved these changes Aug 14, 2018

View reviewed changes

Geod24 force-pushed the json branch from 162b043 to d314290 Compare August 14, 2018 05:32

jacob-carlborg reviewed Aug 14, 2018

View reviewed changes

Geod24 force-pushed the json branch 2 times, most recently from 7cc187a to b5641b3 Compare August 14, 2018 07:03

Geod24 force-pushed the json branch from b5641b3 to d861ff9 Compare August 14, 2018 09:02

jacob-carlborg reviewed Aug 14, 2018

View reviewed changes

Geod24 closed this Aug 14, 2018

Geod24 reopened this Aug 14, 2018

wilzbach closed this Aug 14, 2018

wilzbach reopened this Aug 14, 2018

Geod24 added 7 commits August 14, 2018 18:00

Remove trailing whitespace

4515917

dmd.utils: Add toDString to slice a C-string

3733295

ModuleDeclaration: Provide a toString method

cb962ff

Move DArray definition to DCompat

d351c61

If we want to use DArray from dmd.root it cannot reside in globals.h, so it is moved to a standalone header.

hdrgen: Add some xxxToString methods to wrap xxxToChars

12ac64c

strlen must die, right ?

Convert the JSON code to use D slices instead of pointers

a586bcf

The conversion is pretty straightforward, but required in order to convert globals. Since globals are still C-style string, a local toDString is used. It can later be removed as globals are converted to D slices.

Geod24 force-pushed the json branch from d861ff9 to a586bcf Compare August 14, 2018 16:00

JinShil approved these changes Aug 15, 2018

View reviewed changes

wilzbach approved these changes Aug 15, 2018

View reviewed changes

JinShil added the Merge:auto-merge label Aug 18, 2018

JinShil merged commit 0f735fe into dlang:master Aug 18, 2018

Geod24 deleted the json branch August 18, 2018 03:54

Uh oh!

Conversation

Geod24 commented Aug 10, 2018

Uh oh!

dlang-bot commented Aug 10, 2018

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Aug 13, 2018

Uh oh!

JinShil left a comment

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Geod24 commented Aug 13, 2018 •

edited

Loading

Geod24 commented Aug 14, 2018 •

edited

Loading