convert vec.c to dvec.d #8257

WalterBright · 2018-05-17T18:34:07Z

No description provided.

dlang-bot · 2018-05-17T18:34:08Z

Thanks for your pull request, @WalterBright!

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

jacob-carlborg · 2018-05-17T18:37:41Z

src/dmd/backend/dvec.d

+ * Source:      $(LINK2 https://github.com/dlang/dmd/blob/master/src/dmd/backend/dvec.d, backend/dvec.d)
+ */
+
+


Should there be a module declaration?

The module name defaults to the file name.

I know that but all other D files in DMD has a module declaration.

jacob-carlborg · 2018-05-17T18:38:20Z

src/dmd/backend/dvec.d

+
+static assert(vec_base_t.sizeof==2 && VECSHIFT==4 ||
+              vec_base_t.sizeof==4 && VECSHIFT==5 ||
+              vec_base_t.sizeof==8 && VECSHIFT==6);


Would be nice with a space around the operators.

jacob-carlborg · 2018-05-17T18:39:32Z

src/dmd/backend/dvec.d

+void vec_set(vec_t v)
+{
+    if (v)
+    {   memset(v, ~0, v[0].sizeof * vec_dim(v));


Braces on their own lines.

jacob-carlborg · 2018-05-17T18:40:49Z

The Visual Studio project file is not updated.

wilzbach · 2018-05-18T01:24:49Z

Regarding the Jenkins failure - the Ocean failure is unrelated.
See dlang/ci#208 (after the next push, Jenkins should be green again).

rainers · 2018-05-18T07:57:16Z

src/dmd/backend/dvec.d

+ * Source:      $(LINK2 https://github.com/dlang/dmd/blob/master/src/dmd/backend/dvec.d, backend/dvec.d)
+ */
+
+module dvec;


Should be module dmd.backend.dvec, otherwise separate compilation fails.

Aye, there's the rub. I cannot make this work for both dmc and dmd, because they use separate directory structures. Upgrading the boot compiler to 2.079 is required. Unfortunately, this has been blocked for a year now, and is currently blocked by:
#8249
#8112

I haven't yet heard a convincing reason why dmc and dmd should not share a common directory structure, e.g. use module name tk.dvec and put it into the respective folder. Not using package names is unlikely to be a good idea for shared modules.

The reason is:

It's a lot of nuisance work for me to crowbar dmc into a totally different directory structure - messing up the git history, changing all the imports, etc.

I'm the one doing the work, and this is the way I want to do the work, because mashing it into multiple directories is unproductive for me.

All the blocking items for making this work are inconsequential and do not negatively impact anything else.

All the blocking items are things we need to do anyway (like support the -mv command). I can hardly be the only one who wants to share the same modules among multiple projects.

assert() support in 2.068 is so heavy it is disabled in dmd, with various negative consequences that show up now and then. The back end relies heavily on asserts being ON. 2.079 has much better support for lightweight assert().

I cannot make this work for both dmc and dmd, because they use separate directory structures.

As I mentioned before, this isn't necessarily an issue. If you really can't be bothered [1] to choose a standard directory structure, just keeping them in the top-level package like this is fine (as long as it is actually used as import dvec;).

All the blocking items for making this work are inconsequential and do not negatively impact anything else.

If one were to e.g. regard -betterC as a badly designed command line switch, then supporting it does negatively impact e.g. GDC, as it adds complexity that can't easily be gotten rid of again.

All the blocking items are things we need to do anyway (like support the -mv command). I can hardly be the only one who wants to share the same modules among multiple projects.

People usually prefer to use non-surprising directory structures for this.

assert() support in 2.068 is so heavy it is disabled in dmd, with various negative consequences that show up now and then. The back end relies heavily on asserts being ON. 2.079 has much better support for lightweight assert().

Just replace assert() by a custom template then. Trivial to do during the translation.

[1] Note that moving src/{ddmd -> dmd} came with many more issues (src/dmd being used by the executable, files also used from GDC/LDC), yet you were happy to agree to that.

I could have had the back end converted a year ago. Instead I spend time on this back and forth over a trivial blocker.

You keep repeating this claim, yet all these "blockers" are trivially worked around. If you were really interested in converting the backend, you could have done just that a year ago. Your earlier statement applies verbatim: "It's also TRIVIAL. I simply cannot understand the resistance to it."

As I've pointed out elsewhere, I believe the only such workaround that would require code changes is that to support "release asserts". Any time you might need to spend writing a sed script/… to convert those assert calls to a custom function is entirely dwarfed by the time it takes to do the conversion itself, or indeed the required changes to all the various build systems that go with it.

So, can we please stop pretending that "I need it to convert the backend to D" is a valid argument in favour of bumping the bootstrap compiler version requirement? If you'd like to change the version requirements to make your life easier, I'm not sure why you would need to make up bogus reasons to ask for that. Your word still carries weight here.

sed scripts

Adding more technical debt is to be avoided, not embraced. Never mind that sed isn't a standard tool on Windows, so I'd have to write a program to do that, and check it into the repositories. I know what I think when I see such things in other projects.

trivial

Yes, I could write a script to copy directory structures around before each compile. Is that something you'd want to do for your project? I prefer to avoid rube goldberg build setups. (Heaven knows the build system is fragile enough as it is.)

pretending

I'm used to doing ugly things to get things to work when I have no choice. I've even resorted to writing programs to patch object files. But there's no reason to do that here. The advantage to owning the tool chain is the fix can be put in the right spot.

Your word still carries weight here.

Then please pull the blocking PRs.

sed scripts

Adding more technical debt is to be avoided, not embraced. Never mind that sed isn't a standard tool on Windows

Replacing assert with my_assert is a one-time thing, to be done along with many other mechanical changes during the C -> D conversion.

trivial

Yes, I could write a script to copy directory structures around before each compile.

No need for that, as explained before. Just keep the backend modules in the root package (as in this PR) if you don't want to deal with directories. That's possibly not the prettiest thing to do, but then you are the one who is arguing for a flat structure.

I'm used to doing ugly things to get things to work when I have no choice.

Imho both -betterC and -mv are such ugly things, but that's somewhat besides the point here. What even are the ugly things that you would need to accept in order to use an old D version?

Naming a non-standard assert something different isn't really one, at least in my opinion. A different name can be gainfully employed to highlight (and explain via a DDoc comment) the special requirements for backend use (small code/always enabled/…).

Then please pull the blocking PRs.

One is failing, and @kinke seems to have a valid suggestion for the other one.

I had my own non-standard assert for years in the back end. I finally removed it along with a lot of other garbage I used to think was a good idea.

One is failing,

Because it's blocked by the other.

and @kinke seems to have a valid suggestion for the other one.

Regardless of the merit of his suggestion, it requires delaying for multiple release cycles.

rainers · 2018-05-18T07:59:10Z

src/vcbuild/dmd_backend.vcxproj.filters

@@ -1,446 +1,443 @@
-<?xml version="1.0" encoding="utf-8"?>


This is probably converting CRLF to LF. This should not happen for the VC project files.

Changed it back to CRLF.

jacob-carlborg · 2018-05-18T08:10:50Z

Is there a reason for changing the name from vec to dvec?

rainers · 2018-05-18T08:31:22Z

src/dmd/backend/dvec.d

+        else
+        {
+            v = cast(vec_t) calloc(dim + 2, vec_base_t.sizeof);
+            assert(v);


This should better be reporting "out of memory", the backend uses err_nomem() for this.

It uses the assert because the file is supposed to be independent of any other project modules. The C++ version it was translated from did this, too.

The C++ version uses mem_calloc/mem_exception from mem.c.

assert(v) has the problem that it is likely to go away in a release build, so if you want no dependencies I'd recommend if (!v) assert(false, "out of memory").

See my other comment about why assert not being disabled is required in the back end.

rainers · 2018-05-18T08:32:05Z

src/dmd/backend/dvec.d

+        else
+        {
+            vc = cast(vec_t) calloc(nbytes, 1);
+            assert(vc);


Same here.

Otherwise, translation looks good.

RazvanN7 · 2018-05-18T09:08:22Z

src/dmd/backend/dvec.d

+/**
+ * Compiler implementation of the
+ * $(LINK2 http://www.dlang.org, D programming language).
+ *


I suggest adding a description of the module. I have no idea what the code in this file does. Also, a more descriptive file name wouldn't hurt

WalterBright · 2018-05-30T19:05:38Z

@CyberShadow can you please upgrade the boot compiler on CyberShadow/DAutoTest to 2.079 so this passes?

CyberShadow · 2018-05-30T22:48:05Z

DAutoTest is already set up to use 2.079 once #8112 is merged.

Do you want to merge this PR before #8112? If so, I could change the detection logic enabling a newer host compiler to check for dvec.d instead of -mv in the makefile.

WalterBright · 2018-05-30T23:53:35Z

Do you want to merge this PR before #8112?

Yes, please. #8112 is not a blocker for this, it's just a canary for needed features in the boot compiler.

dlang/dmd#8257 (comment)

CyberShadow · 2018-05-31T14:20:35Z

Done, should take effect on the next rebuild.

WalterBright · 2018-05-31T22:58:56Z

@CyberShadow thanks! It's working now, ready to pull!

JinShil

There are a lot of valid concerns voiced in the review of this PR, but Walter has addressed most of it and made his decision. I think it's time to move on; some things can even be addressed after the conversion to D.

quickfur · 2018-06-03T03:29:56Z

This PR broke my local dmd build, which bootstraps with 2.069:

../generated/linux/release/64/backend.a(dvec.o): In function `_D4dvec9VecGlobal9terminateMUNbNiZv':
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:65: undefined reference to `_D4dvec8__assertFiZv'
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:67: undefined reference to `_D4dvec7__arrayZ'
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:76: undefined reference to `_D4dvec7__arrayZ'
../generated/linux/release/64/backend.a(dvec.o): In function `_D4dvec9VecGlobal8allocateMUNbNimZPm':
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:87: undefined reference to `_D4dvec7__arrayZ'
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:89: undefined reference to `_D4dvec7__arrayZ'
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:106: undefined reference to `_D4dvec8__assertFiZv'
../generated/linux/release/64/backend.a(dvec.o): In function `_D4dvec9VecGlobal3dupMUNbNixPmZPm':
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:131: undefined reference to `_D4dvec7__arrayZ'
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:133: undefined reference to `_D4dvec7__arrayZ'
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:138: undefined reference to `_D4dvec8__assertFiZv'
../generated/linux/release/64/backend.a(dvec.o): In function `_D4dvec9VecGlobal4freeMUNbNiPmZv':
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:162: undefined reference to `_D4dvec7__arrayZ'
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:163: undefined reference to `_D4dvec7__arrayZ'
../generated/linux/release/64/backend.a(dvec.o): In function `vec_setbit':
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:269: undefined reference to `_D4dvec8__assertFiZv'
../generated/linux/release/64/backend.a(dvec.o): In function `vec_clearbit':
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:280: undefined reference to `_D4dvec8__assertFiZv'
../generated/linux/release/64/backend.a(dvec.o): In function `vec_testbit':
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:299: undefined reference to `_D4dvec8__assertFiZv'
../generated/linux/release/64/backend.a(dvec.o): In function `vec_andass':
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:348: undefined reference to `_D4dvec8__assertFiZv'
/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:349: undefined reference to `_D4dvec8__assertFiZv'
../generated/linux/release/64/backend.a(dvec.o):/mnt/1/usr/src/d/dmd/src/dmd/backend/dvec.d:355: more undefined references to `_D4dvec8__assertFiZv' follow
collect2: error: ld returned 1 exit status
--- errorlevel 1
make[1]: *** [posix.mak:475: ../generated/linux/release/64/dmd] Error 1
make[1]: Leaving directory '/mnt/1/usr/src/d/dmd/src'
make: *** [posix.mak:8: all] Error 2

Checking out the revision before this PR was merged fixes the problem.

WalterBright · 2018-06-03T06:16:17Z

You'll need to upgrade your boot compiler.

WalterBright requested review from MartinNowak, dnadlinger, ibuclaw and wilzbach as code owners May 17, 2018 18:34

jacob-carlborg reviewed May 17, 2018

View reviewed changes

WalterBright force-pushed the dvec.d branch 2 times, most recently from c0f5172 to bba39f1 Compare May 17, 2018 20:27

JohanEngelen mentioned this pull request May 17, 2018

fix Issue 18868 - nondeterministic static ctor/dtor #8255

Merged

rainers reviewed May 18, 2018

View reviewed changes

src/dmd/backend/dvec.d

else

{

vc = cast(vec_t) calloc(nbytes, 1);

assert(vc);

Copy link

Member

rainers May 18, 2018

Choose a reason for hiding this comment

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

Same here.

Otherwise, translation looks good.

RazvanN7 reviewed May 18, 2018

View reviewed changes

WalterBright added the Merge:Blocked label May 18, 2018

WalterBright force-pushed the dvec.d branch from bba39f1 to fa7e17c Compare May 18, 2018 16:37

WalterBright force-pushed the dvec.d branch 3 times, most recently from 0d6decd to f313eee Compare May 30, 2018 09:16

CyberShadow added a commit to CyberShadow/ae that referenced this pull request May 31, 2018

ae.sys.d.manager: Update 2.079 host DMD version condition

9232fad

dlang/dmd#8257 (comment)

convert vec.c to dvec.d

96b384b

WalterBright force-pushed the dvec.d branch from f313eee to 96b384b Compare May 31, 2018 21:50

JinShil approved these changes May 31, 2018

View reviewed changes

JinShil removed the Merge:Blocked label May 31, 2018

WalterBright merged commit 6817246 into dlang:master Jun 1, 2018

WalterBright deleted the dvec.d branch June 1, 2018 01:23

JinShil added the D Conversion label Jun 4, 2018

		* Source: $(LINK2 https://github.com/dlang/dmd/blob/master/src/dmd/backend/dvec.d, backend/dvec.d)
		*/

Uh oh!

Uh oh!

convert vec.c to dvec.d #8257

convert vec.c to dvec.d #8257

Conversation

WalterBright commented May 17, 2018

Uh oh!

dlang-bot commented May 17, 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!

jacob-carlborg commented May 17, 2018

Uh oh!

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

dnadlinger May 18, 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!

dnadlinger May 19, 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!

rainers May 18, 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!

jacob-carlborg commented May 18, 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!

WalterBright commented May 30, 2018

Uh oh!

CyberShadow commented May 30, 2018

Uh oh!

WalterBright commented May 30, 2018

Uh oh!

CyberShadow commented May 31, 2018

Uh oh!

WalterBright commented May 31, 2018

Uh oh!

wilzbach commented May 18, 2018 •

edited

Loading

dnadlinger May 18, 2018 •

edited

Loading

dnadlinger May 19, 2018 •

edited

Loading

rainers May 18, 2018 •

edited

Loading

JinShil left a comment •

edited

Loading