Rework #8542 wrt. deterministic IDs via generateIdWithLoc()

[kinke]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @kinke! 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#8633"

[kinke]

Custom hashing of Loc is necessary because of mixins, where the const(char)* Loc.filename pointers diverge, e.g.:

test/runnable/test18880.d-mixin-7 (0x558d3e709930)
test/runnable/test18880.d-mixin-7 (0x558d3e70a120)
test/runnable/test18880.d-mixin-7 (0x558d3e70a3f0)

Btw, 2.081.1 isn't able to compile master, that's pretty bad (I went back to 2.077).

[kinke]

Some remarks:

1. Hashing Loc is case-sensitive wrt. file path on Windows too, unlike opEquals(). So that's not really correct.
2. The existing Loc.equals() method ignoring charnum without explicit -vcolumns is pretty weird IMO; it'd be better renamed to make that clear.
3. Loc.filename should probably be a proper dmd.root.filename.FileName struct (contains a single const(char)* pointer). And FileName should probably get a proper opEquals() and toHash() instead of the Loc. If it contained a D string instead of the C one, it would probably just work out of the box without explicit implementations, at the (memory) cost of an additional size_t per Loc; that is, if we ditched the case-insensitiveness on Windows.
4. The generated IDs aren't really unique per module, but rather per filename (same as current master; there should be no functional changes wrt. generated IDs with this PR). The mixin example above illustrates that (=> artificial filenames with extra suffix). Not sure if there can thus still be collisions between the actual module (foo.d) and a mixin (foo.d-mixin-<N>); base identifier, line and column would have to match for that to happen.

[ibuclaw]

If it contained a D string instead of the C one, it would probably just work out of the box without explicit implementations, at the (memory) cost of an additional size_t per Loc.

I can't see this happening, Loc is a critical object used everywhere. Increasing its size is not really an available option.

[Geod24]

I can't see this happening, Loc is a critical object used everywhere. Increasing its size is not really an available option.

Well that's exactly the goal I've been working towards recently...

[ibuclaw]

Well that's exactly the goal I've been working towards recently...

I'll just leave this review here: #7821 (comment)

[jacob-carlborg]

I can't see this happening, Loc is a critical object used everywhere. Increasing its size is not really an available option.

Isn't Loc mostly passed by const ref? Or is that not your concern?

BTW, in Clan the corresponding type is only 32 bit large. The documentation says:

"Technically, a source location is simply an offset into the manager's view of the input source"

[1] https://clang.llvm.org/doxygen/classclang_1_1SourceLocation.html

[ibuclaw]

Isn't Loc mostly passed by const ref? Or is that not your concern?

It's not how it's passed around, increasing the size of Loc increases the size of every object in the front end.

If we want memory consumption to go in any direction, we would want to keep it down.

BTW, in Clan the corresponding type is only 32 bit large. The documentation says:

The same is also true for GCC's location_t.

Having a location map might be reasonable to ensure that each location is only allocated once. You could make it more useful however by storing location ranges - start and end of all tokens and expressions.

I digress from the topic of this PR however...

[kinke]

toHash() and opEquals() are now consistent (both case-sensitive on Windows). I don't really want to create an uppercase copy each time the hash is requested on Windows, and assume that once we operate with Loc instances, there are no case-insensitive duplicate filenames to worry about. That assumption is probably not valid for generic FileNames, that's why I left it in Loc. equals() should probably be case-sensitive then too actually.

If you think modifying ubiquitous Loc may have too much impact, I can move custom hashing and equivalence to the Key struct.

[jacob-carlborg]

FYI, macOS also has a case insensitive filesystem by default.

[kinke]

Ah, interesting. The FileName.equals() function used here only has a Windows special case.

[kinke]

I had to use ref here, although it makes no sense in this case. To be fair, I haven't tried returning void and incrementing the ref directly.

[kinke]

Btw, thanks @n8sh for this AA.update(), I was looking for it in the docs (didn't want to hash twice for insertion), found it, implemented it, and then figured it's new in 2.082. ;)

[kinke]

Rebased + squashed. Can someone remove the WIP label please?

[jacob-carlborg]

Can someone remove the WIP label please?

Done.

[kinke]

@thewilsonator: Please try to push this. It was only supposed to rectify the IMO horrible code introduced with the important fix in #8542; I don't see why it has to rot here for more than half a year now.

If you think modifying ubiquitous Loc may have too much impact, I can move custom hashing and equivalence to the Key struct.

That 'offer' still stands and is IMO the only question mark; I'll be on vacation though.

[thewilsonator]

I've got it in the AGM at dconf to make Loc an offset into a source manager, that would make the problem of making Loc bigger moot.