Rework #8542 wrt. deterministic IDs via generateIdWithLoc()

Author: kinke

src/dmd/globals.d

@@ -13,6 +13,7 @@
 module dmd.globals;
 
 import core.stdc.stdint;
+import core.stdc.string;
 import dmd.root.array;
 import dmd.root.filename;
 import dmd.root.outbuffer;
@@ -469,6 +470,24 @@ nothrow:
                FileName.equals(filename, loc.filename);
     }
 
+    extern (D) bool opEquals(Loc loc) const pure
+    {
+        return charnum == loc.charnum &&
+               linnum == loc.linnum &&
+               FileName.equals(filename, loc.filename);
+    }
+
+    extern (D) hash_t toHash() const pure
+    {
+        import dmd.root.hash : mixHash;
+
+        auto hash = hashOf(linnum);
+        hash = mixHash(hash, hashOf(charnum));
+        if (filename !is null)
+            hash = mixHash(hash, hashOf(filename[0 .. strlen(filename)]));
+        return hash;
+    }
+
     /******************
      * Returns:
      *   true if Loc has been set to other than the default initialization

src/dmd/identifier.d

@@ -125,18 +125,6 @@ nothrow:
 
     extern (C++) __gshared StringTable stringtable;
 
-    /**
-       A secondary string table is used to guarantee that we generate unique
-       identifiers per module. See generateIdWithLoc and issues
-       https://issues.dlang.org/show_bug.cgi?id=16995
-       https://issues.dlang.org/show_bug.cgi?id=18097
-       https://issues.dlang.org/show_bug.cgi?id=18111
-       https://issues.dlang.org/show_bug.cgi?id=18880
-       https://issues.dlang.org/show_bug.cgi?id=18868
-       https://issues.dlang.org/show_bug.cgi?id=19058.
-     */
-    private extern (C++) __gshared StringTable fullPathStringTable;
-
     static Identifier generateId(const(char)* prefix)
     {
         __gshared size_t i;
@@ -166,75 +154,36 @@ nothrow:
      */
     extern (D) static Identifier generateIdWithLoc(string prefix, const ref Loc loc)
     {
-        import dmd.root.filename: absPathThen;
-
-        // see below for why we use absPathThen
-        return loc.filename.absPathThen!((absPath)
+        // generate `<prefix>_L<line>_C<col>`
+        OutBuffer idBuf;
+        idBuf.writestring(prefix);
+        idBuf.writestring("_L");
+        idBuf.print(loc.linnum);
+        idBuf.writestring("_C");
+        idBuf.print(loc.charnum);
+
+        /**
+         * Make sure the identifiers are unique per module. See issues
+         * https://issues.dlang.org/show_bug.cgi?id=16995
+         * https://issues.dlang.org/show_bug.cgi?id=18097
+         * https://issues.dlang.org/show_bug.cgi?id=18111
+         * https://issues.dlang.org/show_bug.cgi?id=18880
+         * https://issues.dlang.org/show_bug.cgi?id=18868
+         * https://issues.dlang.org/show_bug.cgi?id=19058.
+         */
+        static struct Key { Loc loc; string prefix; }
+        __gshared uint[Key] counters;
+
+        const key = Key(loc, prefix);
+        if (auto pCounter = key in counters)
         {
+            idBuf.writestring("_");
+            idBuf.print((*pCounter)++);
+        }
+        else
+            counters[key] = 1;
 
-            // this block generates the "regular" identifier, i.e. if there are no collisions
-            OutBuffer idBuf;
-            idBuf.writestring(prefix);
-            idBuf.writestring("_L");
-            idBuf.print(loc.linnum);
-            idBuf.writestring("_C");
-            idBuf.print(loc.charnum);
-
-            // This block generates an identifier that is prefixed by the absolute path of the file
-            // being compiled. The reason this is necessary is that we want unique identifiers per
-            // module, but the identifiers are generated before the module information is available.
-            // To guarantee that each generated identifier is unique without modules, we make them
-            // unique to each absolute file path. This also makes it consistent even if the files
-            // are compiled separately. See issues:
-            // https://issues.dlang.org/show_bug.cgi?id=16995
-            // https://issues.dlang.org/show_bug.cgi?id=18097
-            // https://issues.dlang.org/show_bug.cgi?id=18111
-            // https://issues.dlang.org/show_bug.cgi?id=18880
-            // https://issues.dlang.org/show_bug.cgi?id=18868
-            // https://issues.dlang.org/show_bug.cgi?id=19058.
-            OutBuffer fullPathIdBuf;
-
-            if (absPath)
-            {
-                // replace characters that demangle can't handle
-                for (auto ptr = absPath; *ptr != '\0'; ++ptr)
-                {
-                    // see dmd.dmangle.isValidMangling
-                    // Unfortunately importing it leads to either build failures or cyclic dependencies
-                    // between modules.
-                    if (*ptr == '/' || *ptr == '\\' || *ptr == '.' || *ptr == '?' || *ptr == ':')
-                        *ptr = '_';
-                }
-
-                fullPathIdBuf.writestring(absPath);
-                fullPathIdBuf.writestring("_");
-            }
-
-            fullPathIdBuf.writestring(idBuf.peekSlice());
-            const fullPathIdLength = fullPathIdBuf.peekSlice().length;
-            uint counter = 1;
-
-            // loop until we can't find the absolute path ~ identifier, adding a counter suffix each time
-            while (fullPathStringTable.lookup(fullPathIdBuf.peekSlice()) !is null)
-            {
-                // Strip the counter suffix if any
-                fullPathIdBuf.setsize(fullPathIdLength);
-                // Add new counter suffix
-                fullPathIdBuf.writestring("_");
-                fullPathIdBuf.print(counter++);
-            }
-
-            // `idStartIndex` is the start of the "true" identifier. We don't actually use the absolute
-            // file path in the generated identifier since the module system makes sure that the fully
-            // qualified name is unique.
-            const idStartIndex = fullPathIdLength - idBuf.peekSlice().length;
-
-            // Remember the full path identifier to avoid possible future collisions
-            fullPathStringTable.insert(fullPathIdBuf.peekSlice(),
-                                       null);
-
-            return idPool(fullPathIdBuf.peekSlice()[idStartIndex .. $]);
-        });
+        return idPool(idBuf.peekSlice());
     }
 
     /********************************************
@@ -305,8 +254,6 @@ nothrow:
 
     static void initTable()
     {
-        enum size = 28_000;
-        stringtable._init(size);
-        fullPathStringTable._init(size);
+        stringtable._init(28_000);
     }
 }

src/dmd/identifier.h

@@ -37,7 +37,6 @@ class Identifier : public RootObject
     int dyncast() const;
 
     static StringTable stringtable;
-    static StringTable fullPathStringTable;
     static Identifier *generateId(const char *prefix);
     static Identifier *generateId(const char *prefix, size_t i);
     static Identifier *idPool(const char *s, unsigned len);

src/dmd/root/filename.d

@@ -926,80 +926,3 @@ version(Windows)
     }
 
 }
-
-version(Posix)
-{
-
-    /**
-    Takes a callable F and applies it to the result of converting
-    `fileName` to an absolute file path (char*)
-
-    Params:
-        fileName = The file name to be converted to an absolute path
-    Returns: Whatever `F` returns.
-    */
-    auto absPathThen(alias F)(const(char)* fileName)
-    {
-        import core.sys.posix.stdlib: realpath, free;
-        auto absPath = realpath(fileName, null /* realpath allocates */);
-        scope(exit) free(absPath);
-        return F(absPath);
-    }
-}
-else
-{
-    /**
-    Takes a callable F and applies it to the result of converting
-    `fileName` to an absolute file path (char*)
-
-    Params:
-        fileName = The file name to be converted to an absolute path
-    Returns: Whatever `F` returns.
-     */
-    auto absPathThen(alias F)(const(char)* fileName)
-    {
-        import core.sys.windows.winnls: WideCharToMultiByte;
-        import core.stdc.stdlib: malloc, free;
-
-        return fileName.extendedPathThen!((wpath) {
-                // first find out how long the buffer must be to store the result
-                const length = WideCharToMultiByte(0,    // code page
-                                                   0,    // flags
-                                                   wpath,
-                                                   -1,   // wpath len, -1 is null terminated
-                                                   null, // multibyte output ptr
-                                                   0,    // multibyte output length
-                                                   null, // default char
-                                                   null, // if used default char
-                );
-
-                if(!length) return F((char*).init);
-
-                char[1024] buf = void;
-
-                scope multibyteBuf = length > buf.length
-                    ? (cast(char*)malloc(length * char.sizeof))
-                    : &buf[0];
-                scope (exit)
-                {
-                    if (multibyteBuf != &buf[0])
-                        free(multibyteBuf);
-                }
-
-                // now store the result
-                const length2 = WideCharToMultiByte(0,    // code page
-                                                    0,    // flags
-                                                    wpath,
-                                                    -1,   // wpath len, -1 is null terminated
-                                                    multibyteBuf,
-                                                    length,
-                                                    null, // default char
-                                                    null, // if used default char
-                );
-
-                assert(length == length2);
-
-                return F(multibyteBuf);
-        });
-    }
-}

src/dmd/root/hash.d

@@ -12,15 +12,19 @@
 
 module dmd.root.hash;
 
+pure:
+nothrow:
+@nogc:
+
 // MurmurHash2 was written by Austin Appleby, and is placed in the public
 // domain. The author hereby disclaims copyright to this source code.
 // https://sites.google.com/site/murmurhash/
-uint calcHash(const(char)* data, size_t len) pure nothrow @nogc
+uint calcHash(const(char)* data, size_t len)
 {
     return calcHash(cast(const(ubyte)*)data, len);
 }
 
-uint calcHash(const(ubyte)* data, size_t len) pure nothrow @nogc
+uint calcHash(const(ubyte)* data, size_t len)
 {
     // 'm' and 'r' are mixing constants generated offline.
     // They're not really 'magic', they just happen to work well.