fix #158 - handle identifier list containing an indexing expression #159

ghost · 2017-07-23T09:51:38Z

No description provided.

ghost · 2017-07-23T10:13:02Z

The fix is wrong, T[0].T[1].T is well parsed but only the last indexer is stored. The problem is that the indexer cannot be parsed in parseIdentifierOrTemplateInstance because it is used everywhere and would have consequences on the others constructs like the expressions.

In background the problem is that some kind of simplifications make the good fix harder than it looks.
https://dlang.org/spec/grammar.html#IdentifierList is not right in dparse.

ghost · 2017-07-23T17:49:32Z

I've made a nicer fix. As expected i had to create a new AST node type, so that each indexer of the chain is stored. As expected too all the tools break: D-Scanner, D-Symbol. The user tools that used Type2 will also break.

There's no other way to do that. I repeat myself but even if the bug was trivial, libdparse simplifies the AST so that things can be re-used (IdentifierOrTemplateChain here). One use case was forgotten and with the fix the IdentifierOrTemplateChain would not be reusable anymore.

Finally, if this get accepted i think that we should schedule this week, two hours during which the other tools could be fixed (because it requires each a time a review, a tag, etc).

…ifierList can be named IdentifierList as in D specs - extern decl content is also an IdentifierList

ghost · 2017-07-25T14:40:18Z

This good now. Summary of the changes:

IdentifierList is renamed to DeclaratorIdentifierList. It was used in alias declarations and in D specs, this is the name that's used: (https://dlang.org/spec/grammar.html#DeclaratorIdentifierList)
Type2 doesn't use an IdentifierOrTemplateInstanceChain but rather what's officialy an IdentifierList and that can include an indexer after each item of the list. This is the issue that's fixed in this PR.
LinkageAttribute also uses this new IdentifierList, following the specs (https://dlang.org/spec/grammar.html#LinkageAttribute).

ghost · 2017-07-25T16:40:47Z

src/dparse/formatter.d

@@ -3867,3 +3903,35 @@ protected:
        "postblit"
    ];
 }
+
+version (unittest)
+void testFormatNode(Node)(string sourceCode)


use this new function to fix #135 and also problems in harbored-mod (which uses dparse formatter apparently...)

codecov-io · 2017-07-25T19:20:05Z

Codecov Report

Merging #159 into master will decrease coverage by 0.29%.
The diff coverage is 80%.

@@            Coverage Diff            @@
##           master     #159     +/-   ##
=========================================
- Coverage   81.68%   81.39%   -0.3%     
=========================================
  Files           7        7             
  Lines        4542     4573     +31     
=========================================
+ Hits         3710     3722     +12     
- Misses        832      851     +19

Impacted Files	Coverage Δ
src/dparse/ast.d	`15.04% <0%> (-0.05%)`	⬇️
src/dparse/parser.d	`87.3% <86.95%> (-0.51%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a19677...d45051b. Read the comment docs.

Hackerpilot · 2017-07-28T19:50:21Z

src/dparse/parser.d

-     *     ;)
+     *      $(RULE identifierOrTemplateInstance)
+     *    | $(RULE identifierOrTemplateInstance) $(LITERAL '.') $(RULE identifierList)
+     *    | $(RULE identifierOrTemplateInstance) $(LITERAL '[') $(RULE assignExpression) $(LITERAL ']') $(LITERAL '.') $(RULE identifierList)


The language spec and I disagree on the idea of "IdentifierList" being only a list of identifiers.

That and the idea of "lists" being separated by commas.

It's a chain, clearly.

I'm not objecting to your change. It's consistent with the spec. I'm just upset with the spec.

There must be "chain" in the name. It's not a list.

The only other use is with extern(c++).

ghost · 2017-07-31T21:49:17Z

src/dparse/formatter.d

+            stuff.accept(this);
+            format(&fmt, stuff);
+            assert(fmt.data.canFind(code), fmt.data);
+            writeln(fmt.data);


gotta remove this writeln.

…ession (#159)" This reverts commit abab226.

ghost · 2017-08-02T01:22:51Z

reverted. The changes are hard to handle.

Basile Burg added 2 commits July 23, 2017 11:50

fix #158 - handle identifier list containing an indexing expression

b0a76ab

remove warn used to test the fix

d50d46d

ghost changed the title ~~fix #158 - handle identifier list containing an indexing expression~~ [WIP] fix #158 - handle identifier list containing an indexing expression Jul 23, 2017

dlang-bot added the WIP label Jul 23, 2017

add the TypeIdentifierList AST node to allow proper multi indexers

5ee0195

forgot to remove this after adding the new node type

b5fd1ff

ghost changed the title ~~[WIP] fix #158 - handle identifier list containing an indexing expression~~ fix #158 - handle identifier list containing an indexing expression Jul 24, 2017

ghost requested a review from Hackerpilot July 24, 2017 11:36

ghost removed the WIP label Jul 24, 2017

- rename IdentifierList to DeclaratorIdentifierList so that TypeIdent…

b91c5be

…ifierList can be named IdentifierList as in D specs - extern decl content is also an IdentifierList

ghost deleted a comment from codecov-io Jul 25, 2017

Basile Burg added 3 commits July 25, 2017 17:09

update the formatter

57134b8

test and fix using the formatter

08b9460

fix BasicType2X broken

e0e9749

ghost deleted a comment from codecov-io Jul 25, 2017

fix leading dot broken

52b8c9d

ghost commented Jul 25, 2017

View reviewed changes

Hackerpilot reviewed Jul 28, 2017

View reviewed changes

rename IdentifierList to TypeIdentifierChain

2ce9605

ghost commented Jul 31, 2017

View reviewed changes

remove writeln used to test

d45051b

ghost merged commit abab226 into master Aug 1, 2017

ghost pushed a commit that referenced this pull request Aug 2, 2017

Revert "fix #158 - handle identifier list containing an indexing expr…

8c836fb

…ession (#159)" This reverts commit abab226.

ghost mentioned this pull request Jan 16, 2019

fix #338 - A class Inheriting from an AliasSeq item is seen as an error #343

Merged

WebFreak001 mentioned this pull request May 4, 2023

Test suite for the formatter #129

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #158 - handle identifier list containing an indexing expression #159

fix #158 - handle identifier list containing an indexing expression #159

ghost commented Jul 23, 2017

ghost commented Jul 23, 2017

ghost commented Jul 23, 2017

ghost commented Jul 25, 2017

ghost Jul 25, 2017 •

edited by ghost

Loading

codecov-io commented Jul 25, 2017 •

edited

Loading

Hackerpilot Jul 28, 2017 •

edited

Loading

Hackerpilot Jul 28, 2017

ghost Jul 28, 2017

Hackerpilot Jul 28, 2017

ghost Jul 28, 2017

ghost Jul 28, 2017 •

edited by ghost

Loading

ghost Jul 31, 2017

ghost commented Aug 2, 2017

fix #158 - handle identifier list containing an indexing expression #159

fix #158 - handle identifier list containing an indexing expression #159

Conversation

ghost commented Jul 23, 2017

ghost commented Jul 23, 2017

ghost commented Jul 23, 2017

ghost commented Jul 25, 2017

ghost Jul 25, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

codecov-io commented Jul 25, 2017 • edited Loading

Codecov Report

Hackerpilot Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Hackerpilot Jul 28, 2017

Choose a reason for hiding this comment

ghost Jul 28, 2017

Choose a reason for hiding this comment

Hackerpilot Jul 28, 2017

Choose a reason for hiding this comment

ghost Jul 28, 2017

Choose a reason for hiding this comment

ghost Jul 28, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jul 31, 2017

Choose a reason for hiding this comment

ghost commented Aug 2, 2017

ghost Jul 25, 2017 •

edited by ghost

Loading

codecov-io commented Jul 25, 2017 •

edited

Loading

Hackerpilot Jul 28, 2017 •

edited

Loading

ghost Jul 28, 2017 •

edited by ghost

Loading