Turn globals.argv0 into a DArray by Geod24 · Pull Request #8575 · dlang/dmd

Geod24 · 2018-08-18T04:42:46Z

This is a step towards converting globals to use D array.

There is currently no way to call a function accepting a D string from C++ (thanks to ABI issues on i386 / Win64) but it does not matter for globals AFAICS.
So I resorted to providing trivial C++ wrapper to the D functions, and gradually removing the wrapper as they become unused, as can be seen in filename.

dlang-bot · 2018-08-18T04:42:46Z

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#8575"

ibuclaw · 2018-08-18T06:38:04Z

src/dmd/mars.d

 extern(C) void printGlobalConfigs(FILE* stream)
 {
-    stream.fprintf("binary    %s\n", global.params.argv0);
+    stream.fprintf("binary    %*s\n", global.params.argv0.length, global.params.argv0.ptr);


%.*s - if memory serves me right.

ibuclaw

Is github loosing my reviews?

ibuclaw · 2018-08-18T07:00:28Z

Is github loosing my reviews?

Answer, yes. This is twice now I've written something and it's gone.

JinShil · 2018-08-18T07:56:46Z

Is github loosing my reviews?

I experienced that yesterday as well; started questioning my sanity.

jacob-carlborg · 2018-08-18T08:53:22Z

src/dmd/frontend.d

    import std.string : fromStringz, toStringz;

-    auto f = findConfFile(dmdFilePath.toStringz, "dmd.conf");
+    auto f = findConfFile(dmdFilePath.toStringz[0 .. dmdFilePath.length], "dmd.conf");


This is converted to a C string then back to a D string. Am I missing something?

It was a leftover. findConfFile was still dependent on \0 termination, as one of the function was not converted to const(char)[]. I converted it and now \0 termination isn't required anymore.

Those are somewhat connected and not worth the hassle of breaking down this commit.

Geod24 · 2018-09-05T07:11:55Z

This finally passes and is ready for review

jacob-carlborg · 2018-09-05T08:01:06Z

src/dmd/frontend.d

        return null;

-    return f.fromStringz.idup;
+    return f.idup;


You can idup a string that is null, which will save you the if statement.

diff --git a/src/dmd/frontend.d b/src/dmd/frontend.d index 93c5a8a94..9ae64a51a 100644 --- a/src/dmd/frontend.d +++ b/src/dmd/frontend.d @@ -79,11 +79,7 @@ string findDMDConfig(const(char)[] dmdFilePath) import dmd.dinifile : findConfFile; import std.string : fromStringz, toStringz; - auto f = findConfFile(dmdFilePath, "dmd.conf"); - if (f is null) - return null; - - return f.idup; + return findConfFile(dmdFilePath, "dmd.conf").idup; } /**

Updated thanks

jacob-carlborg · 2018-09-05T08:02:49Z

src/dmd/link.d

            Strings* paths = FileName.splitPath(getenv("PATH"));
-            if (auto p = FileName.searchPath(paths, "link.exe", false))
-                return p;
+            if (auto p = FileName.searchPath(paths, "link.exe"[], false))


Why is this slice needed?

Otherwise the call is ambiguous, since we have a const(char)* and a const(char)[] overload, and manifest constant convert to both.
In a later PR (because this is already quite big) I'll remove the const(char)* overload

This is more of a proof of concept for future conversion. Here's a few principles used to do the conversion from `T*` to `T[]`: - If it's a string, the character at `s[$]` must be `'\0'`. It allows code to freely go back and forth between `char*` and `char[]`. - Move the implementation to extern(D) and slices, and provide C++ wrapper. - Stay on topic: those changes only focus on one parameter / module / function, because it's very easy to end up with a giant change that doesn't work. - If `'\0'` termination is required (e.g. calling a C function), changing from `char*` to `char[]` should can be done using `toCStringThen`.

jacob-carlborg · 2018-09-05T08:13:42Z

src/dmd/root/filename.d

+    }
+
+    /// Ditto
+    extern (D) static int exists(const(char)[] name)


Should probably return an enum instead of an int, but that might be something for a separate PR.

jacob-carlborg · 2018-09-05T08:14:54Z

src/dmd/root/filename.d

    {
        import core.sys.posix.stdlib: realpath, free;
-        auto absPath = realpath(fileName, null /* realpath allocates */);
+        char* absPath = fileName.toCStringThen!((fn) => realpath(&fn[0], null /* realpath allocates */));


It is freed later, so I'd have to cast away the const.

Ah, my bad.

ghost · 2018-12-25T11:55:06Z

This causes a regression : https://issues.dlang.org/show_bug.cgi?id=19510.

Geod24 · 2018-12-25T15:48:30Z

~~I'll look at it~~

EDIT: Or not, since you submitted #9142

ghost · 2018-12-25T17:01:07Z

BTW, sorry since i discovered that your not responsible for the regression. It's more a Windows API call that was not used correctly... the problem was only revealed by your changes.

Geod24 requested review from MartinNowak, WalterBright, dnadlinger, ibuclaw, rainers and wilzbach as code owners August 18, 2018 04:42

Geod24 requested a review from jacob-carlborg August 18, 2018 04:43

Geod24 force-pushed the argv0-slice branch 6 times, most recently from 1400745 to 5e9fe05 Compare August 18, 2018 06:46

ibuclaw reviewed Aug 18, 2018

View reviewed changes

Geod24 force-pushed the argv0-slice branch 2 times, most recently from da80538 to 71e023f Compare August 18, 2018 06:53

ibuclaw reviewed Aug 18, 2018

View reviewed changes

Geod24 force-pushed the argv0-slice branch 3 times, most recently from eef083e to dcd7960 Compare August 18, 2018 07:14

jacob-carlborg reviewed Aug 18, 2018

View reviewed changes

Geod24 force-pushed the argv0-slice branch 6 times, most recently from 15a277c to 13a29aa Compare August 20, 2018 00:16

Geod24 added 12 commits September 5, 2018 15:38

utils: Make toDString inout

dc15aff

Convert FileName.absolute to use D slices

35ccf38

Convert FileName.exists to use D slice

0f6c988

Convert FileName.ext to use D slices

53cb00c

Convert FileName.equalsExt to D slice

d7dd3ee

Convert FileName.defaultExt to use D slice

011f065

Convert FileName.forceExt to D slice

337ea2e

Convert FileName.removeExt to use D slice

302d3e9

Convert FileName.name to use D slice

7005000

Convert FileName.path to use D slice

fdfc2f2

Convert FileName.replaceName to use D slice

a627237

Convert the rest of the path functions to D slice

94dd5e3

Those are somewhat connected and not worth the hassle of breaking down this commit.

Geod24 force-pushed the argv0-slice branch from e5eb2f0 to 6041408 Compare September 5, 2018 06:39

Geod24 removed the Review:WIP Work In Progress - not ready for review or pulling label Sep 5, 2018

jacob-carlborg reviewed Sep 5, 2018

View reviewed changes

Geod24 force-pushed the argv0-slice branch from 6041408 to 19b8e31 Compare September 5, 2018 08:11

Geod24 force-pushed the argv0-slice branch from 19b8e31 to 22f30bf Compare September 5, 2018 08:12

jacob-carlborg reviewed Sep 5, 2018

View reviewed changes

jacob-carlborg approved these changes Sep 5, 2018

View reviewed changes

Geod24 mentioned this pull request Sep 6, 2018

Filename: Convert extension-related code to use D slices #8672

Closed

RazvanN7 approved these changes Sep 6, 2018

View reviewed changes

jacob-carlborg added the Merge:auto-merge label Sep 6, 2018

dlang-bot merged commit 61fe641 into dlang:master Sep 6, 2018

Geod24 deleted the argv0-slice branch September 6, 2018 22:30

Uh oh!

Conversation

Geod24 commented Aug 18, 2018

Uh oh!

dlang-bot commented Aug 18, 2018

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

JinShil commented Aug 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

ghost commented Dec 25, 2018

Uh oh!

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

Uh oh!

Uh oh!

ghost commented Dec 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ibuclaw commented Aug 18, 2018 •

edited

Loading

Geod24 Sep 5, 2018 •

edited

Loading

Geod24 commented Dec 25, 2018 •

edited

Loading