Skip to content

[DDMD] Switch to D source and delete C++ source#4923

Merged
MartinNowak merged 1 commit intodlang:masterfrom
yebblies:switch
Aug 23, 2015
Merged

[DDMD] Switch to D source and delete C++ source#4923
MartinNowak merged 1 commit intodlang:masterfrom
yebblies:switch

Conversation

@yebblies
Copy link
Contributor

@WalterBright @MartinNowak

Do we need the switch script in the repo beforehand? I'm thinking not.

I think this is ready to go when it passes the autotester.

@MartinNowak
Copy link
Member

Do we need the switch script in the repo beforehand? I'm thinking not.

Yes we do, it's important for this to work that people can rebase onto both sides, the C code and the D code. Everything else means you'll have to manually merge your changes.
Please pull #4922 first.

@yebblies
Copy link
Contributor Author

Don't close this, I'm using it for autotesting.

@yebblies yebblies reopened this Aug 23, 2015
@MartinNowak
Copy link
Member

Don't close this, I'm using it for autotesting.

Sorry, I just don't want that anyone merges this.

@yebblies
Copy link
Contributor Author

Sorry, I just don't want that anyone merges this.

I understand. So in your opinion, is it enough to merge #4922 then this?

@MartinNowak
Copy link
Member

So in your opinion, is it enough to merge #4922 then this?

Yes should work though I just updated my more complete makefile change.
MartinNowak@6871cb4.
Also you need to update travis.sh now, see MartinNowak@a6a8ba9.

@yebblies
Copy link
Contributor Author

Yes should work though I just updated my more complete makefile change.
MartinNowak/dmd@6871cb4.
Also you need to update travis.sh now, see MartinNowak/dmd@a6a8ba9.

Those can be done after the switch.

@MartinNowak
Copy link
Member

Auto-merge toggled on

@WalterBright
Copy link
Member

Auto-merge toggled on

Woo-hoo!

MartinNowak added a commit that referenced this pull request Aug 23, 2015
[DDMD] Switch to D source and delete C++ source
@MartinNowak MartinNowak merged commit 88ec9d8 into dlang:master Aug 23, 2015
@MartinNowak
Copy link
Member

Woo-hoo!

Yes, thanks again @yebblies for making this happen.

Please let's not rush into major rewrites right away. We still have to port some possible 2.068.1 fixes and don't want to loose all PRs. I'd say our main focus for 2.069.0 should lie on making ddmd really fast and ship it with less bugs than the last release.

There is no changelog entry btw ;-).

@yebblies yebblies deleted the switch branch August 23, 2015 05:12
@WalterBright
Copy link
Member

I did rush into making an announcement. I've been so wanting to do that!

@CyberShadow
Copy link
Member

This broke the doc autotester.

Why was this merged despite the autotester failing?

@MartinNowak
Copy link
Member

Why was this merged despite the autotester failing?

Because the doc autotester has been somewhat flaky recently, so I guess everybody just started to ignore it.

@CyberShadow
Copy link
Member

everybody

You mean the person who clicked the merge button? ;)

The build logs are viewable to anyone, and the failure reason is easy to find.

Should we revert this, then?

@yebblies
Copy link
Contributor Author

Should we revert this, then?

No.

The build logs are viewable to anyone, and the failure reason is easy to find.

It looks like it's not finding the C++ lib, does the test suite pass on that machine?

@CyberShadow
Copy link
Member

That machine does not run the DMD test suite.

@yebblies
Copy link
Contributor Author

I figured, I'm asking can it? It looks like some host configuration issue. It should be able to use dmd to link C++ and D together if CC is set to g++.

@MartinNowak
Copy link
Member

Yes, it doesn't link against libstdc++, try to link with -L--verbose or -Wl,--verbose.

@CyberShadow
Copy link
Member

LDFLAGS is not referenced anywhere in posix.mak now, maybe that's the reason?

@CyberShadow
Copy link
Member

This fixes it:

diff --git a/src/posix.mak b/src/posix.mak
index aac3017..a0d27c7 100644
--- a/src/posix.mak
+++ b/src/posix.mak
@@ -293,10 +293,10 @@ backend.a: $(BACK_OBJS)

 ifdef ENABLE_LTO
 dmd: $(DMD_SRCS) $(ROOT_SRCS) newdelete.o $(GLUE_OBJS) $(BACK_OBJS) verstr.h
-       CC=$(HOST_CC) $(HOST_DMD_RUN) -of$@ $(MODEL_FLAG) -vtls -J. -d $(DFLAGS) $(filter-out verstr.h,$^)
+       CC=$(HOST_CC) $(HOST_DMD_RUN) -of$@ $(MODEL_FLAG) -vtls -J. -d $(DFLAGS) $(filter-out verstr.h,$^) $(addprefix -L,$(LDFLAGS))
 else
 dmd: $(DMD_SRCS) $(ROOT_SRCS) newdelete.o glue.a backend.a verstr.h
-       CC=$(HOST_CC) $(HOST_DMD_RUN) -of$@ $(MODEL_FLAG) -vtls -J. -d $(DFLAGS) $(filter-out verstr.h,$^)
+       CC=$(HOST_CC) $(HOST_DMD_RUN) -of$@ $(MODEL_FLAG) -vtls -J. -d $(DFLAGS) $(filter-out verstr.h,$^) $(addprefix -L,$(LDFLAGS))
 endif

 clean:

@MartinNowak
Copy link
Member

LDFLAGS is not referenced anywhere in posix.mak now, maybe that's the reason?

Mmh, right dmd doesn't link against libstdc++, I wonder why works on other platform.
Shouldn't linking with g++ automatically add -lstdc++?

@MartinNowak
Copy link
Member

dmd already links against -lpthread and -lm, so please only add -lstdc++

@CyberShadow
Copy link
Member

I'll just remove all references to LDFLAGS then?

dmd already links against -lpthread and -lm, so please only add -lstdc++

Where? I don't see anything relevant in git grep pthread

@CyberShadow
Copy link
Member

Where? I don't see anything relevant in git grep pthread

Oh, you mean dmd when invoked does that. OK.

@CyberShadow
Copy link
Member

#4934

@CyberShadow
Copy link
Member

This pull request caused a ~80% reduction of compilation peak heap usage for an empty program on http://digger.k3.1azy.net/trend/ !

Woah, what happened here? Was the "bump the pointer" allocator replaced with the D GC?

@CyberShadow
Copy link
Member

It also caused a 17% increase in linker memory usage, why? Are the object files it emits not the same?

@CyberShadow
Copy link
Member

This pull request broke some tests on http://digger.k3.1azy.net/trend/ because some code DMD emits is not Valgrind-friendly, issue filed: https://issues.dlang.org/show_bug.cgi?id=14952

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants