Add location to user defined attributes by jacob-carlborg · Pull Request #9359 · dlang/dmd

jacob-carlborg · 2019-02-13T11:00:26Z

All symbols should have location information. This is important especially when the compiler is used as a library.

dlang-bot · 2019-02-13T11:00:27Z

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

jacob-carlborg · 2019-02-13T11:02:17Z

~~Depends on: #9350~~. #9350 has been merged.

src/dmd/astbase.d

PetarKirov · 2019-02-13T19:21:17Z

Needs a rebase, otherwise looking good.

jacob-carlborg · 2019-02-13T19:34:58Z

This needs more work.

jacob-carlborg · 2019-02-14T19:35:26Z

I think this is ready to go now. I turned out it required a bit more code then I initially thought. My original tests were flawed. Now all the tests have the UDA own its line to make sure it's the actual location of the UDA AST node that is used and not the AST node of the declaration.

src/dmd/parse.d

thewilsonator

How does this handle multiple UDAs?

jacob-carlborg · 2019-02-16T08:48:15Z

How does this handle multiple UDAs?

Hmm, that's a good question. I'll add a test to find out.

src/dmd/parse.d

WalterBright · 2019-02-16T23:00:37Z

src/dmd/parse.d

     * Parse const/immutable/shared/inout/nothrow/pure postfix
     */
-    StorageClass parsePostfix(StorageClass storageClass, AST.Expressions** pudas)
+    StorageClass parsePostfix(StorageClass storageClass, AST.Expressions** pudas, Loc* udaLoc)


Is there a reason to use * rather than ref?

used with null here

It's not actually relying on it being null. Supplying a temporary for the parameter will work fine, and it will remove the null dependence.

As far as I know, using an out parameter would require to declare a variable at the call site, which would not be used afterwards in a couple of cases.

That is correct. However, on the plus side:

It reduces the complexity of the code by eliminating the conditional logic.

It eliminates use of a pointer, thereby eliminating potential memory corruption.

It eliminates the need to document that the argument may be null.

It eliminates the need for the user to check that there's a null check on every dereference on the pointer.

out self-documents that this will be initialized by the function, not the caller.

The compiler optimizer knows (5) too, though currently it does not make use of such information.

Hence it is a better practice to use out.

WalterBright · 2019-02-16T23:02:03Z

src/dmd/parse.d


            case TOK.at:
                {
+                    if (udaLoc)


Avoid null check by using out parameter.

src/dmd/parse.d

All symbols should have location information. This is important especially when the compiler is used as a library.

jacob-carlborg · 2019-02-17T20:35:06Z

How does this handle multiple UDAs?

This seems a bit more complicated.

jacob-carlborg · 2019-02-18T11:14:11Z

How does this handle multiple UDAs?

This seems a bit more complicated.

As far as I can see the parse will combine multiple UDA declarations in the source code into a single AST node, UserAttributeDeclaration. Currently for a test with multiple UDAs on multiple lines, I'll get the line number of the last UDA. There is no AST node to attach the location to. The expressions inside the UDA will contain location information but that will start where the expression start, that is, it will be missing the @( from the UDA. I think to preserve the location information the AST need to look different.

@thewilsonator @WalterBright any advice?

RazvanN7 · 2021-12-16T15:30:17Z

Superseeded by: #12524

jacob-carlborg requested review from RazvanN7 and andralex as code owners February 13, 2019 11:00

jacob-carlborg mentioned this pull request Feb 13, 2019

Add customizable diagnostic reporter for the lexer and parser #9350

Merged

jacob-carlborg added the Merge:Blocked label Feb 13, 2019

thewilsonator reviewed Feb 13, 2019

View reviewed changes

src/dmd/astbase.d Outdated Show resolved Hide resolved

jacob-carlborg added Review:WIP Work In Progress - not ready for review or pulling and removed Merge:Blocked labels Feb 13, 2019

jacob-carlborg force-pushed the uda-loc branch from 57ae3ad to e48a971 Compare February 13, 2019 19:30

jacob-carlborg force-pushed the uda-loc branch from e48a971 to cf2dee2 Compare February 14, 2019 12:13

jacob-carlborg removed the Review:WIP Work In Progress - not ready for review or pulling label Feb 14, 2019

jacob-carlborg force-pushed the uda-loc branch 3 times, most recently from 4ac65f5 to 812f3cd Compare February 14, 2019 19:32

thewilsonator reviewed Feb 16, 2019

View reviewed changes

src/dmd/parse.d Show resolved Hide resolved

thewilsonator reviewed Feb 16, 2019

View reviewed changes