• Mihaela Chirea (31/31) Nov 07 2020 Hello!
• Imperatorn (3/12) Nov 07 2020 Nice. Keep up the good work
• Jacob Carlborg (17/23) Nov 08 2020 Absolutely not. I feel like I have already mentioned this several times:...
• Mihaela Chirea (9/23) Nov 09 2020 StaticForeach[1] is not an AST node. It's just an object used to
• Jacob Carlborg (7/10) Nov 10 2020 Ok, I see. All AST nodes should have a location. I recommend

Mihaela Chirea <chireamihaela99 gmail.com> writes:

Hello!

Here is an update of the progress made since the last update [1]:
- Brought all DMD as a library additions hidden behind a version 
under DMDLIB: CallbackAPI seemed to be the only one fitting this 
description so far [2]
- Added the start location to CPPMangleDeclaration, 
CPPNamespaceDeclaration and LinkDeclaration [3]

Currently, the CPPNamespaceDeclaration location starts at 
`extern`, just like Nspace, but moving it to the exact start of 
the name is possible. Would you consider this a better option?

The remaining nodes:
- StorageClassDeclaration
- DeprecatedDeclaration
- StaticForeachDeclaration: this one contains a StaticForeach 
which already starts at `static` so I'm not sure if adding it 
here as well would be necessary
- UserAttributeDeclaration: Jacob Carlborg made an attempt in 
this direction a while ago [4], I'll look into it and see what 
could be done in this regard

Next steps:
- Add locations to the remaining nodes
- Add token length

The tasks I planned for this milestone can be found here [5]

#SAOC2020

[1] 
https://forum.dlang.org/post/fodopjdbdxqclclsdcrn forum.dlang.org
[2] https://github.com/dlang/dmd/pull/11923
[3] https://github.com/dlang/dmd/pull/11934
[4] https://github.com/dlang/dmd/pull/9359
[5] 
https://forum.dlang.org/post/kfcxcjauafpcrmfuajam forum.dlang.org

Imperatorn <johan_forsberg_86 hotmail.com> writes:

On Saturday, 7 November 2020 at 11:28:34 UTC, Mihaela Chirea 
wrote:
 Hello!

 Here is an update of the progress made since the last update 
 [1]:
 - Brought all DMD as a library additions hidden behind a 
 version under DMDLIB: CallbackAPI seemed to be the only one 
 fitting this description so far [2]
 - Added the start location to CPPMangleDeclaration, 
 CPPNamespaceDeclaration and LinkDeclaration [3]

 [...]

Nice. Keep up the good work

Jacob Carlborg <doob me.com> writes:

On 2020-11-07 12:28, Mihaela Chirea wrote:

 Currently, the CPPNamespaceDeclaration location starts at `extern`, just 
 like Nspace, but moving it to the exact start of the name is possible. 
 Would you consider this a better option?

Absolutely not. I feel like I have already mentioned this several times: 
The start position needs to point to the first byte of the AST node. If 
you want to get the location of some specific part of a node, you should 
access the smallest sub node and then re-lex the source range the sub 
node occupies (when source ranges are available). Then you can get the 
location of the specific token you're interested in, like the 
identifier. At this point you already know the semantic meanings of the 
tokens, so it shouldn't be any problem.

 - StaticForeachDeclaration: this one contains a StaticForeach which 
 already starts at `static` so I'm not sure if adding it here as well 
 would be necessary

All AST nodes should have a location. Otherwise more special casing in 
the code is required when operating on the nodes. Do you know why there 
are two nodes? Do they have the same meaning?

Either you can duplicate the location, or can (possibly) declare a 
method in `StaticForeachDeclaration` which forwards to the location in 
`StaticForeach`.

-- 
/Jacob Carlborg

Mihaela Chirea <chireamihaela99 gmail.com> writes:

 The start position needs to point to the first byte of the AST 
 node. If you want to get the location of some specific part of 
 a node, you should access the smallest sub node and then re-lex 
 the source range the sub node occupies (when source ranges are 
 available). Then you can get the location of the specific token 
 you're interested in, like the identifier. At this point you 
 already know the semantic meanings of the tokens, so it 
 shouldn't be any problem.

That makes sense. Thanks for explaining!

 - StaticForeachDeclaration: this one contains a StaticForeach 
 which already starts at `static` so I'm not sure if adding it 
 here as well would be necessary

 All AST nodes should have a location. Otherwise more special 
 casing in the code is required when operating on the nodes. Do 
 you know why there are two nodes? Do they have the same meaning?

StaticForeach[1] is not an AST node. It's just an object used to 
keep the common parts of the StaticForeachDeclaration[2] and 
StaticForeachStatement[3] nodes.

[1] https://github.com/dlang/dmd/blob/master/src/dmd/cond.d#L95
[2] 
https://github.com/dlang/dmd/blob/master/src/dmd/attrib.d#L1114
[3] 
https://github.com/dlang/dmd/blob/master/src/dmd/statement.d#L1532

Jacob Carlborg <doob me.com> writes:

On Monday, 9 November 2020 at 18:32:38 UTC, Mihaela Chirea wrote:

 StaticForeach[1] is not an AST node. It's just an object used 
 to keep the common parts of the StaticForeachDeclaration[2] and 
 StaticForeachStatement[3] nodes.

Ok, I see. All AST nodes should have a location. I recommend 
adding methods to `StaticForeachDeclaration` and 
`StaticForeachStatement` which forwards to the location stored in 
`StaticForeach`.

--
/Jacob Carlborg