Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ -> D #4884

Closed
wants to merge 6 commits into from
Closed

C++ -> D #4884

wants to merge 6 commits into from

Conversation

MartinNowak
Copy link
Member

  • remove all C++ sources that were replaced by D sources
  • adjust idgen to generate id.d instead of id.c
  • remove magicport
  • change dmd target in makefiles to build from D sources

@MartinNowak
Copy link
Member Author

Mmh, git rename detection doesn't seem to work. This means the change will make invalidate every PR and make it hard to track history. Any ideas how to help git to detect the renames?

@9rnsr
Copy link
Contributor

9rnsr commented Aug 13, 2015

Git does not have true file rename track system. Whether a file rename happens or not, is detected by the concordance rate of contents. And C++ to D conversion modifies almost 100% of source code lines in a file.

@MartinNowak
Copy link
Member Author

And C++ to D conversion modifies almost 100% of source code lines in a file.

I think git's threshold if 50% by default. So maybe if we omit the indentation changes in the first commit, more files can be detected as renames.

@MartinNowak
Copy link
Member Author

Let's try to find out, why the objc test fails @yebblies.

@dnadlinger
Copy link
Member

The earlier LDC error message you posted indicates an ABI mismatch of some sorts in the compiler, which might or might not be related.

@MartinNowak
Copy link
Member Author

The earlier LDC error message you posted indicates an ABI mismatch of some sorts in the compiler, which might or might not be related.

That was for objc.di and objc_stub.d. This one is just for objc.d and it's also in a different method.

@WalterBright
Copy link
Member

I suggest abandoning using git rename. Just add these files as new files, and remove the .c ones. The only thing that would make me reluctant to remove the .c ones is if it will affect the diffs of the unmerged PRs. Having the diffs break would make it more difficult to redo them.

- remove all C++ sources that were replaced by D sources
- adjust idgen to generate id.d instead of id.c
- remove magicport
- change dmd target in makefiles to build from D sources
@MartinNowak MartinNowak force-pushed the cpp_to_d branch 2 times, most recently from 7b33795 to ab8b4ed Compare August 14, 2015 00:51
@WalterBright
Copy link
Member

Auto-merge toggled on.

Off to Scotland! https://www.youtube.com/watch?feature=player_detailpage&v=qxDJMn-534Y#t=104

- use ENABLE_RELEASE=1 for -O -release -inline
- there was a 16-byte offset to the
  first value for the D class

- use struct to get rid of vtbl
  also makes more sense for that simple data type
@MartinNowak
Copy link
Member Author

Apparently we need to split std.algorithm unittest building, see dlang/phobos#3553.

@yebblies
Copy link
Member

Auto-merge toggled off

@yebblies
Copy link
Member

Yeah, I forgot about the win64 failure.

The only thing that would make me reluctant to remove the .c ones is if it will affect the diffs of the unmerged PRs. Having the diffs break would make it more difficult to redo them.

Having git pick them up as renames is not going to help much anyway. Pull requests can be ported over by rebasing on top of the last commit before this is merged and running the converter to get the D version of the patch.

@MartinNowak
Copy link
Member Author

Pull requests can be ported over by rebasing on top of the last commit before this is merged and running the converter to get the D version of the patch.

Can you please write a more detailed explanation of that in the wiki and/or newsgroup.

@MartinNowak
Copy link
Member Author

Auto-merge toggled on

@MartinNowak
Copy link
Member Author

Or did you turn off the auto-merge on purpose @yebblies?

@yebblies
Copy link
Member

Auto-merge toggled off

@yebblies
Copy link
Member

Sorry, I did just turn it off because of the failure, but I think this still needs a couple of changes to make updating pull requests easier.

@9rnsr
Copy link
Contributor

9rnsr commented Aug 14, 2015

Ideally, each ddmd release will be built by the previous official release. The only reason for 2.067 is a temporary thing to accommodate LDC/GDC building ddmd.

Hmm, OK. I think you meant the bootstrap process should succeed in each dmd commits.
So one more question: If a new merged dmd PR breaks the bootstrap process by accident, how that would be detected? For example, after the ddmd started, if I commit a problematic D code that hits issue 14815 into dmd repo, the generated ddmd.exe will be broken silently. Do we already have fail-safe method to avoid such the problem?

@WalterBright
Copy link
Member

how that would be detected?

That's what the test suite is for. I know it is inadequate, but it's the only answer.

14815 can also be avoided by not having static arrays of objects with destructors, and I don't think the dmd source code has any of that anyway.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 14, 2015

Do we already have fail-safe method to avoid such the problem?

Has the autotester been updated to do three-stage builds?

The complete failsafe should be:

  1. Host ddmd builds the PR and runs testsuite + unittests (this is already done).
  2. PR ddmd builds itself and runs the testsuite + unittests
  3. PR ddmd number 2 builds itself and we check md5sums match the previous bootstrap.

I can't think of any other way to prevent silent corruption.

@WalterBright
Copy link
Member

@ibuclaw is right. (I misunderstood.) I do that with DMC++. It's effective.

@9rnsr
Copy link
Contributor

9rnsr commented Aug 14, 2015

That's what the test suite is for.

Yet not known bugs cannot be detected by the test suite.

14815 can also be avoided by not having static arrays of objects with destructors,

It's workaround. While using 2.067 as the host compiler, all of dmd committers should know that and reject all PRs that will add it.

I don't think the dmd source code has any of that anyway.

I'm talking about the situations after the ddmd will be actually started.

@9rnsr
Copy link
Contributor

9rnsr commented Aug 14, 2015

Has the autotester been updated to do three-stage builds?
The complete failsafe should be:

@ibuclaw Also to me, the mechanically testable suite is enough. Is the process already in makefile? (I couldn't find it in win32.mak) I think we should develop and ready it before ddmd started.

@9rnsr
Copy link
Contributor

9rnsr commented Aug 14, 2015

IMHO, we core dmd committers should learn additional prior knowledge to start ddmd development. Does someone write text about that? If exists, I'd like to read it.

My most big worry is that, the current dmd is not yet stable enough to me. Small but not less major/critical bugs exist (let's search bugzilla issues). Big language design/behavior issues (e.g. issue 313/314) is not yet fixed. When we'll start to rely on current D(dmd) behavior, we should also consider the yet not fixed bugs and problematic behaviors in current D. I'm afraid that it would become additional maintenance cost for our core dmd developers.

I'm interest with the dmd development by using D.
But I'm yet not sure it will be actual benefit in this time.

@WalterBright
Copy link
Member

@9rnsr we cannot recommend that other people use D if we are afraid to use it for the compiler.

@9rnsr
Copy link
Contributor

9rnsr commented Aug 14, 2015

@WalterBright Indeed... OK, I accept the changes toward ddmd. So, my last concern is the mechanical check system suggested by Iain. I'll add a trello card for that.

@WalterBright
Copy link
Member

Thank you, @9rnsr

@9rnsr
Copy link
Contributor

9rnsr commented Aug 14, 2015

I'll add a trello card for that.

Done: https://trello.com/c/4tefvo4i/51-add-three-stage-builds-for-ddmd

@WalterBright
Copy link
Member

@yebblies ping?

@yebblies
Copy link
Member

Now to automate the switch step.

@WalterBright
Copy link
Member

This used to pass all the tests, now no longer. What's the current state of things? Ping @yebblies ping @MartinNowak

@MartinNowak
Copy link
Member Author

Daniel did most of my changes and more in separate PRs, so this won't merge any longer.
Apparently he's working on some script to automate the translation now.

@yebblies
Copy link
Member

I guess it can be done with 1) run conversion, 2) apply diff to makefiles?

Can we cut down on the diff size and just change the rule for dmd.exe, then clean up after the switch?

@WalterBright
Copy link
Member

Whatever it takes to make this happen. It's been 5 days now :-(

@yebblies
Copy link
Member

Haven't forgotten, just busy with real world work.

@WalterBright
Copy link
Member

@yebblies I understand that real world work can interfere. Should @MartinNowak simply rebase this so it works again and move forward with that?

@yebblies
Copy link
Member

@WalterBright We've already been over why this branch is not enough.

@MartinNowak
Copy link
Member Author

@yebblies what's left to do? Can we help somehow?

@yebblies
Copy link
Member

@MartinNowak Can you see anything missing from this commit? If not I'll turn it into a pull request.

yebblies@588ecd7

@MartinNowak
Copy link
Member Author

@MartinNowak Can you see anything missing from this commit? If not I'll turn it into a pull request.

Yes, it's missing a script/makefile rule that deletes the *.c files and stages the new *.d files instead.
In fact you want something dify HEAD~4..mybranch so you can easily rebase your branch onto the new D based master. Let me quickly write something for that.

@MartinNowak
Copy link
Member Author

@MartinNowak Can you see anything missing from this commit? If not I'll turn it into a pull request.

I had some issues with ROOT_OBJS and root.a b/c of the single file compilation. I ended up making a D_ROOT_SRCS list and compiled that directly into the binary, look at my makefiles.

@MartinNowak
Copy link
Member Author

You're deleting async.c w/o replacement. Intended?

@MartinNowak
Copy link
Member Author

You need to update .gitignore to remove the *.d files

@yebblies
Copy link
Member

Yes, I've got a script that I used to generate that commit. I still need to get the auto-update working, which is a little trickier.

I had some issues with ROOT_OBJS and root.a b/c of the single file compilation. I ended up making a D_ROOT_SRCS list and compiled that directly into the binary, look at my makefiles.

I don't understand, what problems? Why doesn't this affect the current DDMD build? Can it be applied to master now before the switch?

You're deleting async.c w/o replacement. Intended?

Yes. It's not needed for a working transition and can be dealt with later.

You need to update .gitignore to remove the *.d files

I don't think that's needed for a working transition.

I'm trying to keep this minimal, with just what's needed so that people can update their PRs. We can make more changes manually after the switch. Are there make targets other than the default and 'auto-tester-build' that are absolutely required for manual or automatic testing?

@MartinNowak
Copy link
Member Author

Yes, I've got a script that I used to generate that commit. I still need to get the auto-update working, which is a little trickier.

See #4922

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.

6 participants