Move addDefaultVersionIdentifiers from mars.d to compiler.d by RazvanN7 · Pull Request #7524 · dlang/dmd

RazvanN7 · 2017-12-26T12:50:47Z

The frontend interface of the compiler library needs to do some initialization which is done in dmd/mars.d. In order to use initialization methods from mars.d there are 3 options:

Duplicate the code in frontend.d
Import mars.d and use the method.
Put it in another file and make frontend.d and mars.d use it

(1), (2) are obviously bad solutions, so this PR implements (3)

dlang-bot · 2017-12-26T12:50:47Z

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

wilzbach · 2017-12-26T13:02:07Z

src/dmd/compiler.d

+
+    VersionCondition.addPredefinedGlobalIdent("D_HardFloat");
+
+    printPredefinedVersions();


Hmmm we should we really do this by default in this method?
How about moving printPredefinedVersions back to mars.d?

It's ok to let it there since the printPredefined versions has all it's code in a block nested by a if (global.params.verbose)

printPredefinedVersions() has nothing to do with addDefaultVersionIdentifiers() - note that no mention of it is made in the Ddoc comment - it should not be called here.

wilzbach · 2017-12-26T13:04:54Z

src/dmd/frontend.d

    else
        static assert(0, "OS not supported yet.");

+    addDefaultVersionIdentifiers();


You can now remove the OS detection above :)
I had to make a copy of this as addDefaultVersionIdentifiers was private and I wanted to keep the changes to a minimum in the initial version.

wilzbach · 2017-12-26T13:06:30Z

src/dmd/compiler.d

+ * Note that if `-defaultlib=` or `-debuglib=` was used,
+ * we don't override that either.
+ */
+void setDefaultLibrary()


Not sure whether this can be re-used by other compilers or the frontend. It looks fairly specific to DMD.

jacob-carlborg · 2017-12-26T13:14:30Z

Note that there's this PR as well #7485.

wilzbach · 2018-01-03T05:22:36Z

@RazvanN7 this needs another rebase

WalterBright · 2018-01-03T08:29:23Z

src/dmd/frontend.d

-    else
-        static assert(0, "OS not supported yet.");
-
+    addDefaultVersionIdentifiers();


printPredefinedVersions() call should go here.

WalterBright · 2018-01-03T08:31:13Z

src/dmd/frontend.d

-    else version(OpenBSD)
-        global.params.isOpenBSD = 1;
-    else
-        static assert(0, "OS not supported yet.");


I see all this is deleted, but I don't see where it is moved to?

Now that addDefaultVersionIdentifiers is called there is no need to do that since the code was duplicated from it initially.

RazvanN7 · 2018-01-05T10:39:38Z

@wilzbach I addressed the issues. I think we're good to go. I had to add -J../res to the parser and lexer library builds otherwise the tests would fail

jacob-carlborg · 2018-01-05T14:12:20Z

I had to add -J../res to the parser and lexer library builds otherwise the tests would fail

Hmm, that's strange. The only file in the res directory is default_ddoc_theme.ddoc which is only used in dmd.doc.

RazvanN7 · 2018-01-05T14:37:12Z

Furthermore, globals.d imports compiler.d which for the sake of addDefaultVersionIdentifiers imports cond.d which is a gateway to other non parser related files. I guess compiler.d should not be part of the parser.

ibuclaw · 2018-01-05T23:37:43Z

I'll remove the import of dmd.compiler from the globals module tomorrow.

ibuclaw · 2018-01-06T14:50:58Z

Shouldn't this be moved to a module like dmd.driver? These functions are useless for other backends.

(Saying that, however, the compiler module won't be shared code).

wilzbach · 2018-01-07T23:49:49Z

src/dmd/frontend.d

-        static assert(0, "OS not supported yet.");
-
+    addDefaultVersionIdentifiers();
+    printPredefinedVersions();


Are we sure that we want to call printPredefinedVersions here at all?
After all, this is the frontend for DMD as a library code.

wilzbach · 2018-01-07T23:52:10Z

@wilzbach I addressed the issues. I think we're good to go. I had to add -J../res to the parser and lexer library builds otherwise the tests would fail

You shouldn't need to add -J../res - this means that your parser.a now imports doc.d.
See also:

#7534 (comment)
#7534 (comment)

Shouldn't this be moved to a module like dmd.driver? These functions are useless for other backends.

dmd.driver sounds a bit misleading. How about mars.compiler, mars.versions or compiler/mars.d?

andralex · 2018-01-22T15:44:36Z

@WalterBright ok with you?

andralex

see merge conflict

andralex · 2018-01-23T01:48:51Z

src/dmd/mars.d

    }
 }
+=======
+>>>>>>> Move addDefaultVersionIdentifiers to frontend.d


looks like some conflict made it into the pr

WalterBright · 2018-01-23T10:06:20Z

src/dmd/compiler.d

+    static if (TARGET_WINDOS)
+    {
+        VersionCondition.addPredefinedGlobalIdent("Windows");
+        global.params.isWindows = true;


This line should really not be here. "Setting predefined identifiers" and "setting the target platform in global" should be in different functions.

WalterBright · 2018-01-23T10:09:35Z

src/dmd/compiler.d

+    VersionCondition.addPredefinedGlobalIdent("D_Version2");
+    VersionCondition.addPredefinedGlobalIdent("all");
+
+    if (global.params.cpu >= CPU.sse2)


Note how this section is using global to set versions, whereas the previous section used TARGET to set it. Using TARGET to set global should be in a separate function, then this function should rely only on global to set versions.

Furthermore, instead of accessing global as a global variable, consider passing a const reference to it to the function.

WalterBright · 2018-01-23T10:14:48Z

src/dmd/compiler.d

+    }
+    else
+    {
+        static assert(0, "fix this");


Fixing a static assert failure is implied, so the message adds no value. "unsupported target" might be better.

RazvanN7 · 2018-01-26T14:17:24Z

Moving those functions to compiler.d is not a good idea. globals.d (which is part of the parser library) imports compiler.d. Moving these functions to compiler.d will introduce semantic dependencies to the parser library. Closing this PR until I will come up with a better solution.

RazvanN7 requested review from CyberShadow, MartinNowak, WalterBright and wilzbach as code owners December 26, 2017 12:50

RazvanN7 mentioned this pull request Dec 26, 2017

Move Compiler structure to own module. #7457

Merged

wilzbach reviewed Dec 26, 2017

View reviewed changes

RazvanN7 force-pushed the Mars_refactor branch 3 times, most recently from d2dcbf6 to 52b32f5 Compare December 28, 2017 10:43

RazvanN7 changed the title ~~Refactor init methods out from mars.d~~ Move addDefaultVersionIdentifiers from mars.d to compiler.d Dec 28, 2017

RazvanN7 force-pushed the Mars_refactor branch 2 times, most recently from ab00dd9 to 26d0f14 Compare December 28, 2017 10:50

dlang-bot added Review:Needs Rebase Review:Needs Work labels Jan 1, 2018

WalterBright reviewed Jan 3, 2018

View reviewed changes

RazvanN7 force-pushed the Mars_refactor branch from 26d0f14 to c78f166 Compare January 5, 2018 10:30

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

RazvanN7 force-pushed the Mars_refactor branch 2 times, most recently from 10c5634 to 70c010f Compare January 5, 2018 10:38

RazvanN7 force-pushed the Mars_refactor branch 3 times, most recently from fd14a30 to eb4ad41 Compare January 5, 2018 11:00

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

wilzbach mentioned this pull request Jan 7, 2018

Move frontend utility functions from the DUB package to dmd.frontend #7485

Merged

wilzbach reviewed Jan 7, 2018

View reviewed changes

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

Move addDefaultVersionIdentifiers to frontend.d

55a73ff

RazvanN7 force-pushed the Mars_refactor branch from eb4ad41 to 55a73ff Compare January 22, 2018 15:47

RazvanN7 requested review from mathias-lang-sociomantic and rainers as code owners January 22, 2018 15:47

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

andralex requested changes Jan 23, 2018

View reviewed changes

src/dmd/mars.d

}

}

=======

>>>>>>> Move addDefaultVersionIdentifiers to frontend.d

Copy link

Member

andralex Jan 23, 2018

Choose a reason for hiding this comment

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

looks like some conflict made it into the pr

WalterBright reviewed Jan 23, 2018

View reviewed changes

RazvanN7 closed this Jan 26, 2018


		VersionCondition.addPredefinedGlobalIdent("D_HardFloat");

		printPredefinedVersions();

Uh oh!

Conversation

RazvanN7 commented Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Dec 26, 2017

Bugzilla references

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 26, 2017

Uh oh!

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

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

Uh oh!

Uh oh!

jacob-carlborg commented Jan 5, 2018

Uh oh!

RazvanN7 commented Jan 5, 2018

Uh oh!

ibuclaw commented Jan 5, 2018

Uh oh!

ibuclaw commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jan 7, 2018

Uh oh!

andralex commented Jan 22, 2018

Uh oh!

andralex left a comment

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!

RazvanN7 commented Jan 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RazvanN7 commented Dec 26, 2017 •

edited

Loading

RazvanN7 commented Jan 5, 2018 •

edited

Loading