[Refactor] Some more C string removal by Geod24 · Pull Request #8592 · dlang/dmd

Geod24 · 2018-08-22T01:14:05Z

A few more commits cleaning up strings while we wait on #8585 .

dlang-bot · 2018-08-22T01:14:09Z

Thanks for your pull request and interest in making D better, @Geod24! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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

jacob-carlborg · 2018-08-22T07:04:05Z

src/dmd/doc.d

 }

-extern (C++) int icmp(const(char)* stringz, const(void)* s, size_t slen)
+private bool icmp(const(char)* stringz, const(char)* s, size_t slen)


Isn’t the name a bit misleading now when it will only make a comparison.

Definitely. Any suggestion ?

equal, iequal, equali, equalCaseInsensitive.

WalterBright · 2018-08-22T09:45:12Z

src/dmd/doc.d

-                null
            ];
-            for (size_t i = 0; table[i]; i++)
+            foreach (const entry; table)


The const is redundant, as table is immutable.

WalterBright · 2018-08-22T09:49:52Z

src/dmd/doc.d

-extern (C++) int icmp(const(char)* stringz, const(void)* s, size_t slen)
+private bool icmp(const(char)* stringz, const(char)* s, size_t slen)
 {
    size_t len1 = strlen(stringz);


Your fixes make all the first arguments to icmp a string, but then they get coerced to pointers by the parameter type, and then strlen is run on them here. Just commit to having string parameters to icmp.

WalterBright · 2018-08-22T09:50:41Z

src/dmd/doc.d

-        return cast(int)(len1 - slen);
-    return Port.memicmp(stringz, cast(char*)s, slen);
+        return false;
+    return Port.memicmp(stringz, s, slen) == 0;


Give up on pointer-based memicmp and just write a loop.

WalterBright · 2018-08-22T09:53:29Z

src/dmd/errors.d

-    loc.filename = filename;
-    loc.linnum = linnum;
-    loc.charnum = charnum;
+    Loc loc = Loc(filename, linnum, charnum);


Geod24 · 2018-08-22T16:54:51Z

Updated:

Addressed Walter's comment
Renamed/refactored icmp into dmd.root.port.Port.iequals and use slicing at call site

jacob-carlborg · 2018-08-22T18:54:05Z

src/dmd/root/port.d

+       Returns:
+         `true` if `s1 == s2` regardless of case
+     */
+    extern(D) static int iequals(const char[] s1, const char[] s2)


Returns int instead of bool

Is if worth mentioning in the documentation that this only works with ASCII (as far as I can tell)?

Geod24 · 2018-08-23T00:07:08Z

Updated according to Jacob's comments:

diff --git a/src/dmd/root/port.d b/src/dmd/root/port.d
index ea52b617b..01ef74d53 100644
--- a/src/dmd/root/port.d
+++ b/src/dmd/root/port.d
@@ -57,6 +57,9 @@ extern (C++) struct Port
     /**
        Compare two slices for equality, in a case-insensitive way

+       Comparison is based on `char` and does not do decoding.
+       As a result, it's only really accurate for plain ASCII strings.
+
        Params:
          s1 = string to compare
          s2 = string to compare
@@ -64,7 +67,7 @@ extern (C++) struct Port
        Returns:
          `true` if `s1 == s2` regardless of case
      */
-    extern(D) static int iequals(const char[] s1, const char[] s2)
+    extern(D) static bool iequals(const(char)[] s1, const(char)[] s2)
     {
         if (s1.length != s2.length)
             return false;

Those constants are not exported to the C++ header, so there is no point to have them extern(C++) __gshared when the can just be immutable.

WalterBright · 2018-08-23T07:37:34Z

src/dmd/doc.d

+                if (Port.iequals(entry, name[0 .. namelen]))
                {
-                    buf.printf("$(DDOC_%s ", table[i]);
+                    buf.printf("$(DDOC_%.*s ", entry.length, entry.ptr);


When using .*s, you must cast the entry.length parameter to int, because entry.length is a size_t.
Also, this change to .*s doesn't buy anything anyway, since the string is guaranteed to be 0 terminated.

I reverted this bit to .ptr

Printing is the main issue I encountered when converting pointers to slices, and I think we need a more convenient way to print slices for it to move forward.

dnadlinger · 2018-08-23T22:59:42Z

src/dmd/root/port.d

+       Returns:
+         `true` if `s1 == s2` regardless of case
+     */
+    extern(D) static bool iequals(const(char)[] s1, const(char)[] s2)


Nitpick: Port was originally intended to encapsulate platform-specific code, hence the name.

I was a bit surprised too, but decided to go with consistency.
Moved iequals to utils

dnadlinger · 2018-08-24T04:12:06Z

src/dmd/dinifile.d

                    if (!writeToEnv(environment, strdup(pn)))
                    {
-                        error(Loc(filename, lineNum, 0), "Use `NAME=value` syntax, not `%s`", pn);
+                        error(filename, lineNum, 0, "Use `NAME=value` syntax, not `%s`", pn);


This is imho a regression in legibility/code quality. If you want to eliminate an overload, I'd prefer getting rid of the non-Loc one.

You're right, I only considered the fact that this overload was used in only one place.
However changing this is a bit more involved and would trigger other changes, so I simply removed this commit from this PR and will do the changes in a separate PR.

dnadlinger · 2018-08-24T04:51:09Z

src/dmd/doc.d

        case ' ':
        case '\t':
        case '\n':
-            continue;


This is personal taste, but I found the continue version easier to read, as it can't be confused with breaking out of the loop.

dnadlinger · 2018-08-24T04:52:46Z

src/dmd/imphint.d

 {
-    return hints.get(cast(string) s[0..strlen(s)], null).ptr;
+    if (auto ptr = s.toDString() in hints)
+        return (*ptr).ptr;


Not sure if anything containing the snippet (*ptr).ptr can qualify as an improvement…

dnadlinger · 2018-08-24T04:54:25Z

src/dmd/link.d

        version (OSX)
        {
-            __gshared const(char)* nmeErrorMessage = "`__Dmain`, referenced from:";
+            static immutable nmeErrorMessage = "`__Dmain`, referenced from:";


Imho a raw pointer is more appropriate as long as it's only used as a C string as it encodes the presence of a null character by convention. Again just a question of style, though.

dnadlinger · 2018-08-24T04:55:39Z

@Geod24: Feel free to auto-merge if you don't want to spend any time discussing stylistics…

This was previously used in doc, however as we move more towards slice, the code could be simplified to the point it had to be removed. Newly added `dmd.utils : iequals` will perform a case-insensitive string comparison of two slices.

Geod24 · 2018-08-24T05:12:11Z

Updated:

diff --git a/src/dmd/imphint.d b/src/dmd/imphint.d
index 95f6319f0..57631750a 100644
--- a/src/dmd/imphint.d
+++ b/src/dmd/imphint.d
@@ -23,8 +23,8 @@ import dmd.utils;
  */
 extern (C++) const(char)* importHint(const(char)* s)
 {
-    if (auto ptr = s.toDString() in hints)
-        return (*ptr).ptr;
+    if (auto entry = s.toDString() in hints)
+        return entry.ptr;
     return null;
 }
diff --git a/src/dmd/doc.d b/src/dmd/doc.d
index 60b9b724d..44fc81782 100644
--- a/src/dmd/doc.d
+++ b/src/dmd/doc.d
@@ -1846,7 +1846,7 @@ extern (D) const(char)[] skipwhitespace(const(char)[] p)
         case ' ':
         case '\t':
         case '\n':
-            break;
+            continue;
         default:
             return p[idx .. $];
         }
diff --git a/src/dmd/link.d b/src/dmd/link.d
index 3334d2e0a..2d67e08e0 100644
--- a/src/dmd/link.d
+++ b/src/dmd/link.d
@@ -112,11 +112,11 @@ version (Posix)
     {
         version (OSX)
         {
-            static immutable nmeErrorMessage = "`__Dmain`, referenced from:";
+            static immutable(char*) nmeErrorMessage = "`__Dmain`, referenced from:";
         }
         else
         {
-            static immutable nmeErrorMessage = "undefined reference to `_Dmain`";
+            static immutable(char*) nmeErrorMessage = "undefined reference to `_Dmain`";
         }
         FILE* stream = fdopen(fd, "r");
         if (stream is null)
@@ -136,7 +136,7 @@ version (Posix)
             const(char)* lastSep = strrchr(buffer.ptr, '\n');
             if (lastSep)
                 buffer[(end = lastSep - &buffer[0])] = '\0';
-            if (strstr(&buffer[0], nmeErrorMessage.ptr))
+            if (strstr(&buffer[0], nmeErrorMessage))
                 nmeFound = true;
             if (lastSep)
                 buffer[end++] = '\n';

Geod24 · 2018-08-24T07:00:05Z

Well this is green with auto-merge but dlang bot doesn't care, so merging manually

Geod24 requested review from MartinNowak, WalterBright, andralex, jacob-carlborg and rainers as code owners August 22, 2018 01:14

Geod24 force-pushed the ddoc-stringification branch from 802e5c8 to 58ecfc5 Compare August 22, 2018 01:18

Geod24 requested review from dnadlinger and ibuclaw as code owners August 22, 2018 01:18

jacob-carlborg reviewed Aug 22, 2018

View reviewed changes

Geod24 force-pushed the ddoc-stringification branch from 58ecfc5 to c5c7bf9 Compare August 22, 2018 07:10

WalterBright reviewed Aug 22, 2018

View reviewed changes

Geod24 force-pushed the ddoc-stringification branch 2 times, most recently from 0c3e225 to 1f5b35d Compare August 22, 2018 16:34

jacob-carlborg reviewed Aug 22, 2018

View reviewed changes

Geod24 force-pushed the ddoc-stringification branch from 1f5b35d to 815d722 Compare August 23, 2018 00:05

Privatize / unexport some manifest constants

410b6a3

Those constants are not exported to the C++ header, so there is no point to have them extern(C++) __gshared when the can just be immutable.

Geod24 force-pushed the ddoc-stringification branch from 815d722 to fc76c62 Compare August 23, 2018 03:23

jacob-carlborg approved these changes Aug 23, 2018

View reviewed changes

WalterBright reviewed Aug 23, 2018

View reviewed changes

Geod24 force-pushed the ddoc-stringification branch from fc76c62 to 4440e8a Compare August 23, 2018 07:41

Geod24 closed this Aug 23, 2018

Geod24 reopened this Aug 23, 2018

dnadlinger reviewed Aug 23, 2018

View reviewed changes

Geod24 force-pushed the ddoc-stringification branch from 4440e8a to 83f93d3 Compare August 24, 2018 01:02

dnadlinger reviewed Aug 24, 2018

View reviewed changes

Geod24 force-pushed the ddoc-stringification branch from 83f93d3 to 3bf2778 Compare August 24, 2018 04:25

dnadlinger reviewed Aug 24, 2018

View reviewed changes

dnadlinger approved these changes Aug 24, 2018

View reviewed changes

Geod24 added 6 commits August 24, 2018 14:08

imphint: Do not cast to string, use const(char)[]

ac168e7

Minor: Convert a __gshared to a static immutable

8af7ca7

Minor: Use Loc ctor instead of memberwise assignment

8e8bb76

doc: Modernize a manifest constant

b5c4d2a

utils: Introduce case insensitive slice comparison

d4e4f6a

This was previously used in doc, however as we move more towards slice, the code could be simplified to the point it had to be removed. Newly added `dmd.utils : iequals` will perform a case-insensitive string comparison of two slices.

doc: Provide a slice overload of skipWhitespace

f55173e

Geod24 force-pushed the ddoc-stringification branch from 3bf2778 to f55173e Compare August 24, 2018 05:09

JinShil approved these changes Aug 24, 2018

View reviewed changes

JinShil added the Merge:auto-merge label Aug 24, 2018

Geod24 added Merge:auto-merge and removed Merge:auto-merge labels Aug 24, 2018

Geod24 merged commit 6a1e255 into dlang:master Aug 24, 2018

Geod24 deleted the ddoc-stringification branch August 24, 2018 07:00

Uh oh!

Conversation

Geod24 commented Aug 22, 2018

Uh oh!

dlang-bot commented Aug 22, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Aug 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Aug 23, 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!

dnadlinger Aug 24, 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!

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 commented Aug 24, 2018

Uh oh!

Geod24 commented Aug 24, 2018

Uh oh!

Geod24 commented Aug 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dnadlinger Aug 24, 2018 •

edited

Loading