Added offset to the token location

[AsterMiha]

I added a field to the Loc structure but to address the concerns raised here these changes should only be available when building for a certain version. At the moment, the version part is commented so the lexer unittest I added could run.
How can I make this test run only when building with the correct version?

[dlang-bot]

Thanks for your pull request and interest in making D better, @AsterMiha! 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 coverage diff by visiting the details link of the codecov check)
• 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 run digger -- build "master + dmd#11823"

[MoonlightSentinel]

How can I make this test run only when building with the correct version?

You can simply add the version here (as long as you are only using unit tests):

dmd/test/tools/unit_test_runner.d

Lines 246 to 260 in 5fda4ea

	auto flags = [
	"-version=NoBackend",
	"-version=GC",
	"-version=NoMain",
	"-version=MARS",
	"-unittest",
	"-J" ~ buildOutputPath,
	"-J" ~ projectRootDir.buildPath("src/dmd/res"),
	"-I" ~ projectRootDir.buildPath("src"),
	"-I" ~ unitTestDir,
	"-i",
	"-main",
	"-of" ~ outputPath,
	"-m" ~ model
	] ~ testFiles ~ runnerPath;

[RazvanN7]

cc @jacob-carlborg How does this look?

[RazvanN7]

One comment that I have is that we should unify all changes under a common switch called "DMDLIB". Therefore, replace the EXTRA_LOCATION switch with DMDLIB and maybe in a subsequent PR replace all other variations on this topic ([1][2])

[1] #11092
[2] #11265

[thewilsonator]

The whole point of using offsets was to reduce the size of Loc and therefore the memory the compiler consumes.
This could be done by an indirection/offset table to map (size_t (or unit) x Table) -> (filename, linenum, charnum).
How this mapping is done is up to you but it should be modular and I'd like to see the numbers before this is merged.

[RazvanN7]

@thewilsonator That is a different point. This change is trying to add offset location for tools that use dmd as a library. As you can see it is hided by a compiler version. The refactoring that you are talking about may be the subject of a different PR and indeed might help save some space, but the purpose of this PR is entirely different.

[RazvanN7]

Actually, thinking more about it, this does seem like a great opportunity for optimization.

[AsterMiha]

The whole point of using offsets was to reduce the size of Loc and therefore the memory the compiler consumes.
This could be done by an indirection/offset table to map (size_t (or unit) x Table) -> (filename, linenum, charnum).
How this mapping is done is up to you but it should be modular and I'd like to see the numbers before this is merged.

Thanks for the suggestion! I will attempt to do this optimization but in a different PR.
Right now I'm working on adding components useful for tools using dmd as a library and since this will be done under a version it shouldn't impact the memory the compiler uses at the moment. So before this refactoring, I will also add the token size (under this version of course).

[jacob-carlborg]

The whole point of using offsets was to reduce the size of Loc and therefore the memory the compiler consumes

@thewilsonator This PR is to store the offset of a token from the start of the source buffer, i.e. byte count. This is required to be able to do source code refactorings.

[jacob-carlborg]

I'm wondering if there are other places in the code that needs to be updated as well. I know there are other places in the code which creates locations, not just the lexer.

[jacob-carlborg]

Looking good, just a few minor things left.

[jacob-carlborg]

Looks good, just the extra tokens left: #11823 (comment).

[JohanEngelen]

If you are interested in optimizing source location tracking, I strongly advise reading up on how Clang does that. It has a very efficient implementation. DMD's "Loc" size = 64 + 32 + 32 = 128 bits. Clang's is 32bits.
https://clang.llvm.org/docs/InternalsManual.html#the-sourcelocation-and-sourcemanager-classes

[AsterMiha]

@JohanEngelen Thank you! I will look into it.

[jacob-carlborg]

LGTM.

[jacob-carlborg]

@AsterMiha in the future, when fixing something in a PR related to a specific comment. Please replay to that comment that the issue has been fixed and resolve the conversation. This will make it easier for reviewers to see that a particular comment has been addressed.