convert list.c to dlist.d by WalterBright · Pull Request #8317 · dlang/dmd

WalterBright · 2018-06-01T02:59:51Z

Git diff doesn't seem to render diffs when files get renamed. :-(

dlang-bot · 2018-06-01T02:59:52Z

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

jacob-carlborg · 2018-06-01T06:30:45Z

Git diff doesn't seem to render diffs when files get renamed. :-(

You could do the rename in on commit and the actual changes in a second commit. The it's possible to diff the individual commits, but perhaps not directly on GitHub.

Also, is there a reason to call it dlist instead of list?

JinShil · 2018-06-02T10:53:50Z

src/dmd/backend/dlist.d

+        while (list_freelist)
+        {
+            list_t list = list_next(list_freelist);
+            list_freelist = list;


The C implementation had a call to list_delete here.

Yes, it does seem to be missing!

JinShil · 2018-06-02T10:54:54Z

src/dmd/backend/dlist.d

+}
+
+
+list_t list_alloc();


Why this declaration?

Leftover from the conversion. Removed.

JinShil · 2018-06-02T11:01:48Z

src/dmd/backend/dlist.d

+ * Returns:
+ *    pointer to that list entry.
+ */
+


I'm not sure why you always have a newline between the DDoc and the code. I recommend removing it, but it's not a problem.

The newline is the style commonly used. Others have started using the no newline approach. Besides, I try to minimize diffs on initial conversions.

JinShil · 2018-06-02T11:06:59Z

src/dmd/backend/dlist.d

+    if (list)
+    {
+        list.next = *plist;
+        list.ptr = ptr;


C implementation used the list_next and list_ptr wrappers. It seems to be the same functionality, but throughout this file you're sometimes using the wrapper and sometimes not. It makes it a little hard to review and the code is inconsistent.

list_next and list_ptr don't work as lvalues in D.

wilzbach

Not much more I can add besides @JinShil's nice review.

wilzbach · 2018-06-04T00:47:03Z

src/dmd/backend/dlist.d

+
+
+list_t list_new() { return cast(list_t)malloc(LIST.sizeof); }
+void list_delete(list_t list) { free(list); }


These two functions are never used in the new dlist.d and haven't been used outside of list.c

They're both used.

wilzbach

Let's move finally forward with this!
I'm just unsure about the commented va_end call.

wilzbach · 2018-06-17T05:38:46Z

src/vcbuild/dmd_backend.vcxproj.filters

    </ClCompile>
-    <ClCompile Include="..\dmd\tk\vec.c">
-      <Filter>dmd\tk</Filter>
-    </ClCompile>


It might make sense to move the vec.h header file to the backend, s.t. D code and header files are "in sync".

wilzbach · 2018-06-17T05:50:40Z

src/dmd/backend/dlist.d

+            pe = &list.next;
+        }
+    }
+    //va_end(ap);


What's the reason for not calling va_end here?
(in C it's called)

There was some problem with it being @nogc and nothrow. core.stdc.stdarg apparently needs some additional work.

Compiles fine for me -> #8367

rainers · 2018-06-17T06:48:27Z

src/dmd/backend/dlist.d

+
+alias void function(void*) @nogc nothrow list_free_fp;
+
+enum FPNULL = cast(list_free_fp)null;


You can avoid the cast with enum list_free_fp FPNULL = null. But defining a new symbol here doesn't really buy a lot in comparison to just using null.

FPNULL dates from the olden days when function pointers were different from regular pointers due to the wacky DOS memory models. It doesn't make sense any more, as you point out, but changing it here violates the #1 rule of translation.

rainers · 2018-06-17T06:49:57Z

src/dmd/backend/dlist.d

+    {
+        list = list_freelist;
+        list_freelist = list_next(list);
+        //mem_setnewfileline(list,file,line);


Keeping this as a comment doesn't really help as all the (conditional) parameter declarations have gone.

rainers · 2018-06-17T06:52:11Z

src/dmd/backend/dlist.d

+struct LIST
+{
+        /* Do not access items in this struct directly, use the         */
+        /* functions designed for that purpose.                         */


Can you make them private now?

Gawd will smite me if I violate the #1 rule of translation.

Note that if I make it private now, then I have to add to this PR all the other files that are modified to make that work. Of course, that will inevitably cause more problems. Such changes need to be separate PRs.

WalterBright · 2018-06-17T07:36:13Z

Fellas, these are good suggestions but please keep in mind the #1 rule of translating code - do not attempt to refactor, fix, enhance, improve, rename, reorganize, etc. the code. Do as little as possible to get it to work. I have learned this bitter lesson over and over and over.

JinShil

do not attempt to refactor, fix, enhance, improve, rename, reorganize, etc. the code

Yeah, let's get this done.

WalterBright requested a review from MartinNowak as a code owner June 1, 2018 02:59

WalterBright requested a review from wilzbach as a code owner June 1, 2018 02:59

WalterBright force-pushed the dlist.d branch 4 times, most recently from 0d2ee2b to ae61b0e Compare June 1, 2018 05:41

JinShil reviewed Jun 2, 2018

View reviewed changes

WalterBright added the D Conversion label Jun 3, 2018

wilzbach reviewed Jun 4, 2018

View reviewed changes

WalterBright force-pushed the dlist.d branch from ae61b0e to e3f8520 Compare June 14, 2018 09:07

convert list.c to dlist.d

c92e765

WalterBright force-pushed the dlist.d branch from e3f8520 to c92e765 Compare June 16, 2018 01:45

wilzbach approved these changes Jun 17, 2018

View reviewed changes

rainers reviewed Jun 17, 2018

View reviewed changes

JinShil approved these changes Jun 17, 2018

View reviewed changes

JinShil added the Merge:auto-merge label Jun 17, 2018

dlang-bot merged commit 4613464 into dlang:master Jun 17, 2018

wilzbach mentioned this pull request Jun 17, 2018

dlist.d cleanup: call va_end in list_build #8367

Merged

WalterBright deleted the dlist.d branch June 17, 2018 19:48



		list_t list_new() { return cast(list_t)malloc(LIST.sizeof); }
		void list_delete(list_t list) { free(list); }


		alias void function(void*) @nogc nothrow list_free_fp;

		enum FPNULL = cast(list_free_fp)null;

Uh oh!

Comments

Conversation

WalterBright commented Jun 1, 2018

Uh oh!

dlang-bot commented Jun 1, 2018

Bugzilla references

Testing this PR locally

Uh oh!

jacob-carlborg commented Jun 1, 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!

Choose a reason for hiding this comment

Uh oh!

wilzbach 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!

wilzbach 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!

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 Jun 17, 2018

Uh oh!

JinShil left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jacob-carlborg commented Jun 1, 2018 •

edited

Loading