convert aa.c to aarray.d #8831

WalterBright · 2018-10-14T04:03:09Z

Since virtual functions are problematic in -betterC, I replaced the hash table inheritance with parametric polymorphism, i.e. templates. Had to add a little bit extra to interface it with C++. Once it's all in D, the inlining should make it significantly faster.

dlang-bot · 2018-10-14T04:03:10Z

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

jacob-carlborg · 2018-10-14T11:14:29Z

Seems like GitHub doesn't understand the rename. If you can make the rename a separate commit it will be much easier to review.

WalterBright · 2018-10-14T20:10:40Z

It's not a rename, it's a rewrite. aarray.d is a new file, aa.c is deleted.

jacob-carlborg · 2018-10-15T07:41:23Z

src/dmd/backend/aarray.d

+
+alias hash_t = size_t;
+
+struct aaA


Looks like this can be private.

Does not follow standard D naming conventions

Please come up with a better name and avoid abbreviations

It's largely copy/pasted from the previous implementation in aa.c. I try to avoid renaming things in the first translated version, for reasons expounded upon several times.

I interpreted your previous comment as this is not a direct translation and therefore added quite a lot of comments that I would not normally do for a direct translation PR.

jacob-carlborg · 2018-10-15T07:41:29Z

src/dmd/backend/aarray.d

+import core.stdc.stdlib;
+import core.stdc.string;
+
+alias hash_t = size_t;


No, it'll be used by anyone using this module.

jacob-carlborg

Many of the public functions are missing documentation or missing standard sections like Returns and Params.

jacob-carlborg · 2018-10-15T07:41:44Z

src/dmd/backend/aarray.d

+    /* value */
+}
+
+struct AArray(TKey, Value)


No documentation.

jacob-carlborg · 2018-10-15T07:41:58Z

src/dmd/backend/aarray.d

+
+struct AArray(TKey, Value)
+{
+    alias Key = TKey.Key;       // key type


No documentation.

jacob-carlborg · 2018-10-15T07:42:02Z

src/dmd/backend/aarray.d

+        destroy();
+    }
+
+    void destroy()


No documentation.

jacob-carlborg · 2018-10-15T07:42:09Z

src/dmd/backend/aarray.d

+        }
+    }
+
+    size_t length()


No documentation.

jacob-carlborg · 2018-10-15T07:42:32Z

src/dmd/backend/aarray.d

+    /*************************************************
+     * Get pointer to value in associative array indexed by key.
+     * Add entry for key if it is not already there.
+     * Returns:


Missing Params section.

jacob-carlborg · 2018-10-15T07:53:44Z

src/dmd/backend/aarray.d

+        if (!nodes)
+            return null;
+
+        const aligned_keysize = aligntsize(Key.sizeof);


Does not follow standard D naming conventions.

jacob-carlborg · 2018-10-15T07:53:52Z

src/dmd/backend/aarray.d

+        if (!nodes)
+            return;
+
+        size_t newbuckets_length = prime_list[$ - 1];


Does not follow standard D naming conventions.

jacob-carlborg · 2018-10-15T07:54:00Z

src/dmd/backend/aarray.d

+                break;
+            }
+        }
+        auto newbuckets = cast(aaA**)calloc(newbuckets_length, (aaA*).sizeof);


Does not follow standard D naming conventions.

jacob-carlborg · 2018-10-15T07:54:25Z

src/dmd/backend/aarray.d

+    return (tsize + size_t.sizeof - 1) & ~(size_t.sizeof - 1);
+}
+
+immutable uint[14] prime_list =


Does not follow standard D naming conventions.

jacob-carlborg · 2018-10-15T07:55:16Z

src/dmd/backend/aarray.d

+/***************************************************************/
+
+// Simple values
+public struct Tinfo(K)


Is this used?

It will be by anyone making a simple hash table with a simple key. Currently it is only used by the test code further down.

andralex

I'll approve because previous code was just as undocumented so in a way adding docs is "optional". @WalterBright please consider adding docs though prior to merging.

resolved the raised issues

WalterBright · 2018-10-17T08:45:25Z

Sigh. I though buildkite was fixed. It's failing again with no log.

jacob-carlborg · 2018-10-17T09:03:47Z

@WalterBright the output is:

[INFO] Running /var/lib/buildkite-agent/builds/buildkite-agent-01-1/dlang/dmd/build/dlang-dub/test/interactive-remove.sh...
Package dub not found for registry at https://code.dlang.org/ (fallback ["registry at http://code.dlang.org/", "registry at https://code-mirror.dlang.io/", "registry at https://code-mirror2.dlang.io/", "registry at https://dub-registry.herokuapp.com/"]): No package candidate found for dub 0.9.20
No package dub was found matching the dependency 0.9.20
Fetching dub 0.9.21...
[ERROR] /var/lib/buildkite-agent/builds/buildkite-agent-01-1/dlang/dmd/build/dlang-dub/test/interactive-remove.sh:10 command failed

Not sure if it's related or expected.

WalterBright · 2018-10-17T10:20:40Z

The semaphoreci one looks spurious as well.

wilzbach · 2018-10-17T11:38:39Z

@WalterBright the output is:

[INFO] Running /var/lib/buildkite-agent/builds/buildkite-agent-01-1/dlang/dmd/build/dlang-dub/test/interactive-remove.sh...
Package dub not found for registry at https://code.dlang.org/ (fallback ["registry at http://code.dlang.org/", "registry at https://code-mirror.dlang.io/", "registry at https://code-mirror2.dlang.io/", "registry at https://dub-registry.herokuapp.com/"]): No package candidate found for dub 0.9.20
No package dub was found matching the dependency 0.9.20
Fetching dub 0.9.21...
[ERROR] /var/lib/buildkite-agent/builds/buildkite-agent-01-1/dlang/dmd/build/dlang-dub/test/interactive-remove.sh:10 command failed

Not sure if it's related or expected.

FYI: dlang/dlang-bot#202

WalterBright added Severity:Refactoring No semantic changes to code D Conversion labels Oct 14, 2018

WalterBright requested review from MartinNowak and wilzbach as code owners October 14, 2018 04:03

WalterBright force-pushed the aarray.d branch 20 times, most recently from 078f410 to de5acdc Compare October 14, 2018 09:52

WalterBright requested review from ibuclaw and dnadlinger as code owners October 14, 2018 09:52

WalterBright force-pushed the aarray.d branch 3 times, most recently from e329eea to 83748af Compare October 14, 2018 10:37

WalterBright force-pushed the aarray.d branch 2 times, most recently from e33ebb0 to 7dc4b3c Compare October 14, 2018 10:43

jacob-carlborg reviewed Oct 15, 2018

View reviewed changes

jacob-carlborg previously requested changes Oct 15, 2018

View reviewed changes

WalterBright force-pushed the aarray.d branch from 7dc4b3c to 6c3a60c Compare October 15, 2018 10:57

andralex approved these changes Oct 17, 2018

View reviewed changes

convert aa.c to aarray.d

35708f1

WalterBright force-pushed the aarray.d branch from 6c3a60c to 35708f1 Compare October 17, 2018 08:24

WalterBright added the Merge:auto-merge label Oct 17, 2018

WalterBright merged commit bedd02a into dlang:master Oct 17, 2018

WalterBright deleted the aarray.d branch October 17, 2018 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert aa.c to aarray.d #8831

convert aa.c to aarray.d #8831

WalterBright commented Oct 14, 2018 •

edited

Loading

dlang-bot commented Oct 14, 2018

jacob-carlborg commented Oct 14, 2018

WalterBright commented Oct 14, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

WalterBright Oct 15, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

WalterBright Oct 15, 2018

jacob-carlborg left a comment

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

jacob-carlborg Oct 15, 2018

WalterBright Oct 15, 2018

andralex left a comment

WalterBright commented Oct 17, 2018

jacob-carlborg commented Oct 17, 2018

WalterBright commented Oct 17, 2018

wilzbach commented Oct 17, 2018

convert aa.c to aarray.d #8831

convert aa.c to aarray.d #8831

Conversation

WalterBright commented Oct 14, 2018 • edited Loading

dlang-bot commented Oct 14, 2018

Bugzilla references

Testing this PR locally

jacob-carlborg commented Oct 14, 2018

WalterBright commented Oct 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob-carlborg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andralex left a comment

Choose a reason for hiding this comment

WalterBright commented Oct 17, 2018

jacob-carlborg commented Oct 17, 2018

WalterBright commented Oct 17, 2018

wilzbach commented Oct 17, 2018

WalterBright commented Oct 14, 2018 •

edited

Loading