Replace C++ syntax check with dummy frontend by ibuclaw · Pull Request #7520 · dlang/dmd

ibuclaw · 2017-12-25T23:53:01Z

On closer inspection the Visitor interface broken by #7397 still wasn't fixed by #7438.

The entire C++ Visitor class has been re-layed out to match the order of virtuals in the D implementation.

Have exported Id::initialize to C++ as fundamental components don't work if you can't call it.

Replaced cxxcheckheaders with cxx-unittest, a small program that links against the front-end in the same way that LDC or GDC would to test that interaction works.

I anticipate to build on this test file later.

dlang-bot · 2017-12-25T23:53:03Z

Thanks for your pull request, @ibuclaw!

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.

ibuclaw · 2017-12-25T23:57:51Z

@MartinNowak - this should go in stable + 2.078, as the C++<->D visitor interface is utterly unusable without this, and breaks all downstream.

ibuclaw · 2017-12-26T00:03:33Z

Looks like I should partially revert #7005 also, as the fix is just lazy.

wilzbach · 2017-12-26T00:04:18Z

@MartinNowak - this should go in stable + 2.078, as the C++<->D visitor interface is utterly unusable without this, and breaks all downstream.

Simple target stable, then it will be part of 2.078 (you can change the targeted branch directly via the GitHub UI, but you will need to do the git rebase too).
Once merge we can easily merge stable back into master.

ibuclaw · 2017-12-26T00:53:37Z

Rebased against stable. I think the tests should pass now once I fixed up ForwardingStatement visit methods - @tgehr.

tgehr · 2017-12-26T06:34:07Z

I'm not sure why #7005 needs to be reverted. (Or why it is at all virtuous to not be "lazy" -- there is exactly one sensible default implementation, therefore #7005 prevents all further bugs like the one it fixed.)

tgehr · 2017-12-26T06:47:55Z

src/dmd/sapply.d

+    override void visit(ForwardingStatement s)
+    {
+        if (s.statement)
+            doCond(s.statement) || applyTo(s.statement);


I don't know the exact purpose of the walkPostorder visitor, but this seems fishy to me as ForwardingStatement is not supposed to have behaviour different from the wrapped statement.

This is correct, as you want to walk over the underlying wrapped statement, not the wrapper itself.

If I have a ForwardingStatement wrapping a LabelStatement, doCond is called with different arguments than if I have only a LabelStatement. Before your changes, the behaviour was identical for both cases. Is this change in behaviour intentional?

Do you have code example that changes behaviour? Or is that just hypothetical?

The order that it walks the AST should be the same as the original.

Should probably add applyTo(s) if the underlying statement is null, but that does not happen in the current.

Running the visitor on a semantically-analyzed version of the following code will result in changed behaviour:

static foreach(i;0..1) L: writeln(i);

L: writeln(i) will be visited by the StoppableVisitor v twice now, even though before it was visited only once.

I don't think applyTo(s) should be called if s.statement is null.

Only if it makes sense to Visit the ForwardingStatement by default. (I don't see why it would.)
Is this Visitor used for anything?

It's used by comeFrom and/or blockExit for determining reachable statements.

Visiting the ForwardingStatement will break the hasCode method in statement.d. (I.e., I think the implementation should stay the same as before.)

Right, that sounds like what I saw last night when seeing unreachable warnings.

So just the one applyTo(s.statement then to elide double visiting.

I think it should just be

if(s.statement) s.statement.accept(this);

(Otherwise, e.g. bodies of while loops inside static foreach will not be visited.)

ibuclaw · 2017-12-26T09:37:47Z

there is exactly one sensible default implementation, therefore #7005 prevents all further bugs like the one it fixed.

The base visitor should just forward onto the next method in the inheritance chain. Any special logic above that should go in overriding visitors.

tgehr · 2017-12-26T09:48:51Z

Well, then the best fix might be to add a DefaultVisitor in-between Visitor and all its subclasses. To forward to visit(Statement) by default is to do the wrong thing by default. Everybody who wants to introduce a new subclass of Visitor now needs to be aware of ForwardingStatement. This is a trap that can lead to bugs.

ibuclaw · 2017-12-26T10:04:06Z

To forward to visit(Statement) by default is to do the wrong thing by default. Everybody who wants to introduce a new subclass of Visitor now needs to be aware of ForwardingStatement. This is a trap that can lead to bugs.

Having it in visitor just hid bugs. As it wasn't forwarding onto Statement in the event that the underlying wrapped statement was null.

If your writing a visitor, that you should be aware of all classes is a given. If the entire point of ForwardingStatement is to not exist outside semantic then that needs to be looked at and fixed. If we can do away with ForwardingStatement in any way (I don't know what it's used for, so I'll arbitrarily suggest adding a Scope flag) then all the better.

tgehr · 2017-12-26T10:13:13Z

Which bugs? If it is sensible to ignore a null Statement, it is sensible to ignore a ForwardingStatement wrapping null.

ForwardingStatement is used to insert wrapped symbols into a different scope than the current one.
It could probably be removed during semantic analysis, if that is a useful thing to do.

ibuclaw · 2017-12-26T10:22:20Z

It could probably be removed during semantic analysis, if that is a useful thing to do.

It seems to me that this is something that should not be leaked outside of the static foreach semantic, so yes.

ibuclaw · 2017-12-26T10:34:34Z

Which bugs? If it is sensible to ignore a null Statement, it is sensible to ignore a ForwardingStatement wrapping null.

If I have a visitor with the following method.

class MV : public Visitor
{
  void visit(Statement *) { puts("have statement"); }
}

Passing this a ForwardingStatement with interior null would print nothing, which is an unexpected behaviour.

tgehr · 2017-12-26T10:41:59Z

I'll try to create a pull request that removes ForwardingStatements after they are no longer required.
Knowing the semantics of ForwardingStatement, that behaviour is expected, but point taken.

ibuclaw · 2017-12-27T09:53:44Z

I moved the ForwardingStatement fix to #7526

ibuclaw · 2017-12-27T22:14:54Z

Cherry-picked other PR into this one.

PetarKirov · 2017-12-28T07:01:14Z

@ibuclaw #7526 has been merged. Rebase?

ibuclaw · 2017-12-28T07:02:43Z

@ZombineDev already rebased.

PetarKirov · 2017-12-28T11:41:09Z

@ZombineDev already rebased.

Hmm, I missed that #7526 was targeting master instead of stable, unlike this PR.

wilzbach · 2017-12-28T12:07:45Z

src/posix.mak

-DMD_FLAGS  := -I$(ROOT) -Wuninitialized
-GLUE_FLAGS := -I$(ROOT) -I$(TK) -I$(C)
+DMD_FLAGS  := -I$(D) -I$(ROOT) -Wuninitialized
+GLUE_FLAGS := -I$(D) -I$(ROOT) -I$(TK) -I$(C)


The common style of this Makefile is to skip the parentheses for one letter variables, i.e. $D

All I have to say is $(C).

wilzbach · 2017-12-28T12:19:53Z

src/tests/cxxfrontend.c

@@ -0,0 +1,199 @@
+/**


Creating the folder src/tests might lead to confusions for newcomers.

I wasn't sure the best place for this, as the main source folder doesn't seem right.

MartinNowak · 2017-12-29T22:03:37Z

Cherry-picked other PR into this one.

Please avoid cherry-picking if possible to not unnecessary merge conflicts.

ibuclaw · 2017-12-30T01:03:03Z

@MartinNowak so having the source in tests is fine?

ibuclaw requested review from CyberShadow, MartinNowak, WalterBright and wilzbach as code owners December 25, 2017 23:53

wilzbach added this to the 2.078.0 milestone Dec 26, 2017

ibuclaw force-pushed the cxxtest branch from e571d07 to 2eace30 Compare December 26, 2017 00:47

ibuclaw changed the base branch from master to stable December 26, 2017 00:48

ibuclaw force-pushed the cxxtest branch 2 times, most recently from d9ed729 to 6ba0644 Compare December 26, 2017 00:52

tgehr reviewed Dec 26, 2017

View reviewed changes

ibuclaw added 6 commits December 27, 2017 23:12

Peel off outer ForwardingStatement after running semantic

4e29a4f

Remove all dead code from ForwardingStatement

09bb96f

Remove special handling of ForwardingStatement in Visitor

fb8f3ec

Fix C++ Visitor interface to match vtbl layout on D side.

6c2cccc

Export Id.initializer as extern(C++)

dac8df7

Replace C++ header syntax checking with test frontend

3923fb2

ibuclaw force-pushed the cxxtest branch from 6ba0644 to 3923fb2 Compare December 27, 2017 22:13

wilzbach reviewed Dec 28, 2017

View reviewed changes

MartinNowak merged commit 9180f36 into dlang:stable Dec 29, 2017

ibuclaw deleted the cxxtest branch December 30, 2017 01:02

PetarKirov mentioned this pull request Jan 5, 2018

[Trivial] add func attribs to class Declaration final helpers #7607

Merged

Uh oh!

Conversation

ibuclaw commented Dec 25, 2017

Uh oh!

dlang-bot commented Dec 25, 2017

Bugzilla references

Uh oh!

ibuclaw commented Dec 25, 2017

Uh oh!

ibuclaw commented Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Dec 26, 2017

Uh oh!

ibuclaw commented Dec 26, 2017

Uh oh!

tgehr commented Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgehr Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgehr Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Dec 26, 2017

Uh oh!

tgehr commented Dec 26, 2017

Uh oh!

ibuclaw commented Dec 26, 2017

Uh oh!

tgehr commented Dec 26, 2017

Uh oh!

ibuclaw commented Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented Dec 26, 2017

Uh oh!

tgehr commented Dec 26, 2017

Uh oh!

ibuclaw commented Dec 27, 2017

Uh oh!

ibuclaw commented Dec 27, 2017

Uh oh!

PetarKirov commented Dec 28, 2017

Uh oh!

ibuclaw commented Dec 28, 2017

Uh oh!

PetarKirov commented Dec 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinNowak commented Dec 29, 2017

Uh oh!

ibuclaw commented Dec 26, 2017 •

edited

Loading

tgehr commented Dec 26, 2017 •

edited

Loading

ibuclaw Dec 26, 2017 •

edited

Loading

tgehr Dec 26, 2017 •

edited

Loading

tgehr Dec 26, 2017 •

edited

Loading

ibuclaw commented Dec 26, 2017 •

edited

Loading