Move frontend utility functions from the DUB package to dmd.frontend by wilzbach · Pull Request #7485 · dlang/dmd

wilzbach · 2017-12-21T15:40:52Z

Following to #7425, this just move these bits to dmd.frontend. Of course, further work is needed to define the API, but I thought having an open PR is the best place to facilitate this discussion.

This is far from perfect, but I won't have time to work much on it until I chat with @RazvanN7.

Anyhow if you have already have an API idea/requirement and want to drop it or just want to subscribe to the discussion, this is the place to be ;-)

dlang-bot · 2017-12-21T15:40:53Z

Thanks for your pull request, @wilzbach!

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.

jacob-carlborg · 2017-12-21T16:24:52Z

What's the idea here, just move common functionality from the test package or to come up new a proper API?

jacob-carlborg · 2017-12-21T16:25:51Z

src/dmd/frontend.d

+    foreach (p; path)
+    {
+        import std.string : toStringz;
+        Strings* a = new Strings();


Why is an array created for each path, why not added the path directly to res?

wilzbach · 2017-12-21T16:36:50Z

What's the idea here, just move common functionality from the test package or to come up new a proper API?

Both, but (B) is definitely the goal. I just moved the bits from the test package over to bootstrap a discussion and avoid this topic sinking in the abyss.
Anyhow I assume that coming up with a proper API from scratch will be tricky and require a couple of iterations (and use cases). So in any case I will put a red reminder in the module header that warns the user about the API's instability. Maybe dmd.experimental.frontend would have been a better place for this module?

jacob-carlborg · 2017-12-21T16:28:19Z

src/dmd/frontend.d

+        .array
+        .sort
+        .uniq
+        .map!buildNormalizedPath;


This whole expression doesn't look very nice. Perhaps split it up with some helper functions/aliases. It's not very clear of what the code is actually doing.

jacob-carlborg · 2017-12-21T16:32:38Z

src/dmd/frontend.d

+shared static this()
+{
+    initDMD();
+}


I'm not sure if this is the best idea if we want to allow multiple instances of the compiler in the future.

jacob-carlborg · 2017-12-21T16:33:32Z

src/dmd/frontend.d

+    import dmd.dinifile : findConfFile;
+    import dmd.errors : fatal;
+    import std.algorithm, std.range, std.regex;
+    import std.stdio : File,  stderr;


There's an extra space after the comma.

jacob-carlborg · 2017-12-21T16:40:34Z

src/dmd/frontend.d

+    import std.algorithm, std.range, std.regex;
+    import std.stdio : File,  stderr;
+
+    auto dmdEnv = env.get("DMD", "dmd");


I think this function needs to be split into several layers building on top of each other. One where it's possible to give a direct path to the config and another which tries to infer it.

jacob-carlborg · 2017-12-21T16:42:36Z

src/dmd/frontend.d

+Params:
+    path = range of imports to add
+*/
+void addImports(T)(T path)


This should have constraints.

jacob-carlborg · 2017-12-21T16:44:00Z

src/dmd/frontend.d

+Returns: the parsed module object
+*/
+// TODO: allow files?
+auto parseModule(string fileName, string code)


I don't think auto should be used here. It returns a simple class and not a template, so I don't think it's necessary.

jacob-carlborg · 2017-12-21T16:45:58Z

src/dmd/frontend.d

+Returns:
+    Pretty printed module as string.
+*/
+auto prettyPrint(Module m)


Same here, write out the actual return type instead of auto.

jacob-carlborg · 2017-12-21T20:42:22Z

So in any case I will put a red reminder in the module header that warns the user about the API's instability. Maybe dmd.experimental.frontend would have been a better place for this module?

I'm don't think that's necessary. I do think the version of the first releases should be 0.x.y, that allows breaking changes at any point according to semantic versioning.

jacob-carlborg · 2017-12-28T09:17:28Z

src/dmd/frontend.d

+
+    if (global.path is null)
+    {
+        Strings* res = new Strings();


This variable doesn't seem to add any value.

jacob-carlborg · 2017-12-28T14:10:16Z

src/dmd/frontend.d

+
+    return File(iniFile, "r")
+        .byLineCopy
+        // search for all -I imports in this file


I still prefer separate functions/lambdas instead of comments.

ibuclaw · 2018-01-06T14:48:57Z

Shouldn't this module be named something like dmd.driver? It's pretty useless for other backends.

wilzbach · 2018-01-07T22:56:21Z

Shouldn't this module be named something like dmd.driver? It's pretty useless for other backends.

It's not intended to be used by any library. It's only intended to be used for the DMD as a library builds - most likely via DUB.

Regarding the name, see: #7425 (comment)
I also don't think the name is perfect, so happy to rename.

wilzbach · 2018-01-07T23:44:04Z

src/dmd/mars.d

 * the `CRuntime` used).
 */
-private void addDefaultVersionIdentifiers()
+void addDefaultVersionIdentifiers()


See #7524 for a better approach

mathias-lang-sociomantic · 2018-01-08T18:50:56Z

src/dmd/frontend.d

-Initializes the DMD compiler
+import dmd.dmodule : Module;
+import std.range.primitives : isInputRange, ElementType;
+import std.traits : isNarrowString;


Thought Phobos is DMD was a no-no ?

This file isn't used by dmd at all - only by the DMD as a library project.
Should we rename the file or move it to somwhere else, s.t. this gets clearer?

I don't see any reason not to use Phobos here, it allows to write the high-level wrappers and stops us from re-inventing the wheel and blowing up the code with inferior, probably buggy implementations of Phobos.

wilzbach · 2018-01-14T19:55:04Z

BTW anything else blocking this PR except for the name of this module?
(it's already there and we can always rename if after this PR though)

Everything is still marked as unstable, so we can always improve/change this, but it should give us sth. to get started.

jacob-carlborg · 2018-01-14T20:09:31Z

src/dmd/frontend.d

+	if ("DMD" in environment)
+		compilers = environment.get("DMD") ~ compilers;
+	auto paths = environment.get("PATH", "").splitter(sep);
+    auto res = compilers.map!(c => paths.map!(p => p.buildPath(c~exe))).joiner.filter!exists;


The indentation is wrong.

wilzbach · 2018-01-17T02:27:48Z

Closing & reopening for Semaphore to learn about this PR.

wilzbach · 2018-01-24T07:31:17Z

Anything else blocking this?

wilzbach · 2018-01-25T16:30:39Z

CC @RazvanN7 are you okay with this too?

RazvanN7 · 2018-01-25T16:56:47Z

LGTM

wilzbach requested a review from WalterBright as a code owner December 21, 2017 15:40

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Dec 21, 2017

wilzbach mentioned this pull request Dec 21, 2017

Expose D frontend via DUB #7425

Merged

wilzbach changed the title ~~[WIP] Move frontend utility functions from the DUB packag to dmd.frontend~~ [WIP] Move frontend utility functions from the DUB package to dmd.frontend Dec 21, 2017

jacob-carlborg reviewed Dec 21, 2017

View reviewed changes

jacob-carlborg suggested changes Dec 21, 2017

View reviewed changes

jacob-carlborg mentioned this pull request Dec 26, 2017

Move addDefaultVersionIdentifiers from mars.d to compiler.d #7524

Closed

jacob-carlborg suggested changes Dec 28, 2017

View reviewed changes

dlang-bot added the Review:Needs Rebase label Jan 1, 2018

wilzbach force-pushed the frontend-code branch from 16bb798 to 4e0bae1 Compare January 4, 2018 11:08

dlang-bot added Review:Needs Rebase and removed Review:Needs Rebase labels Jan 4, 2018

dlang-bot added the Review:Needs Work label Jan 7, 2018

wilzbach force-pushed the frontend-code branch from 4e0bae1 to 13bdd35 Compare January 7, 2018 22:53

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Jan 7, 2018

wilzbach force-pushed the frontend-code branch from 13bdd35 to f102704 Compare January 7, 2018 23:42

wilzbach commented Jan 7, 2018

View reviewed changes

wilzbach force-pushed the frontend-code branch from f102704 to 7bc3dcc Compare January 8, 2018 03:05

wilzbach changed the title ~~[WIP] Move frontend utility functions from the DUB package to dmd.frontend~~ Move frontend utility functions from the DUB package to dmd.frontend Jan 8, 2018

wilzbach removed the Review:WIP Work In Progress - not ready for review or pulling label Jan 8, 2018

wilzbach force-pushed the frontend-code branch 2 times, most recently from 1ff3d42 to 6794c43 Compare January 8, 2018 18:41

wilzbach requested a review from MartinNowak as a code owner January 8, 2018 18:41

wilzbach requested review from mathias-lang-sociomantic and rainers as code owners January 8, 2018 18:41

mathias-lang-sociomantic reviewed Jan 8, 2018

View reviewed changes

wilzbach force-pushed the frontend-code branch from 6794c43 to d464620 Compare January 13, 2018 05:06

wilzbach force-pushed the frontend-code branch from d464620 to 2af57e8 Compare January 14, 2018 19:56

jacob-carlborg suggested changes Jan 14, 2018

View reviewed changes

wilzbach force-pushed the frontend-code branch from 2af57e8 to 1fcdbde Compare January 17, 2018 02:26

wilzbach closed this Jan 17, 2018

wilzbach reopened this Jan 17, 2018

wilzbach force-pushed the frontend-code branch from 1fcdbde to 502ad98 Compare January 22, 2018 15:24

Move frontend utility functions from the DUB packag to dmd.frontend

ff6df1c

wilzbach force-pushed the frontend-code branch from 502ad98 to ff6df1c Compare January 22, 2018 15:41

jacob-carlborg approved these changes Jan 24, 2018

View reviewed changes

jacob-carlborg added the Merge:auto-merge label Jan 25, 2018

dlang-bot merged commit 289b79e into dlang:master Jan 25, 2018

wilzbach deleted the frontend-code branch January 26, 2018 00:00

Uh oh!

Conversation

wilzbach commented Dec 21, 2017

Uh oh!

dlang-bot commented Dec 21, 2017

Bugzilla references

Uh oh!

jacob-carlborg commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Dec 21, 2017

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!

jacob-carlborg commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Jan 6, 2018

Uh oh!

wilzbach commented Jan 7, 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!

wilzbach commented Jan 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jan 17, 2018

Uh oh!

wilzbach commented Jan 24, 2018

Uh oh!

wilzbach commented Jan 25, 2018

Uh oh!

RazvanN7 commented Jan 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants