Skip to content

refactor expression.h: remove fields unused by backend#5975

Closed
WalterBright wants to merge 1 commit intodlang:masterfrom
WalterBright:unneeded-doth
Closed

refactor expression.h: remove fields unused by backend#5975
WalterBright wants to merge 1 commit intodlang:masterfrom
WalterBright:unneeded-doth

Conversation

@WalterBright
Copy link
Member

Back end .h files should be on a need-to-know basis.

@UplinkCoder
Copy link
Member

Does this work ?
won't the class layout be seen diffrently _

@UplinkCoder
Copy link
Member

Ah I get it. you removed all the members.

@rainers
Copy link
Member

rainers commented Jul 28, 2016

I don't think this is a good idea. It makes it harder for other compiler backends (or other tools when using the front end as a library in the future).

For example, LDC uses code like ((IdentifierExp *)exp)->ident, too. There are other types being used in static_casts. I didn't find any of the ones changed here, though.

IIRC the plan was to autogenerate the C++ header files in the long run.

@yebblies
Copy link
Contributor

Ehhh this doesn't seem like a good idea.

@jpf91
Copy link
Contributor

jpf91 commented Jul 28, 2016

pinging some backend developers: @ibuclaw @kinke @klickverbot

@kinke
Copy link
Contributor

kinke commented Jul 28, 2016

I generally dislike .d and .h going out of sync. I find it absolutely natural that the C++ header corresponds to the D implementation. Hiding members this way for C++ bindings (and ending up with different type sizes etc.) seems a particularly bad idea to me.

@dnadlinger
Copy link
Contributor

Yep, this seems like a bad idea. Now, these changes might not break LDC in particular (I didn't check), but:

  • During the last couple of months, we've been bitten by mismatching header files more than once. We should really be auto-generating them (@yebblies, what about just using your hacked up compiler version in a cron-job/Travis check for now while still keeping the generated files in-tree, if the holdup is finding a way of distributing it?). Even if this change is "benign", it still lets the .h/.d files drift further out of sync, and it stands to reason that this makes maintenance harder.
  • In general, you'd have to check all three compilers whether they are using any of the declarations before removing them. Sometimes, we are forced to pull more information from the AST than the DMD glue code does, because it tends to contain mismatching types, ambiguous lowerings, etc. that just happen to work with the way DMD's glue code is written (which, in turn, is partially a consequence of elem trees containing little type information).

I'm all for a cleaner AST interface to the glue layer (with a well-defined "canonical" form of the AST, and a verification pass to check that expression types match up, etc.), but this seems like the wrong way towards this goal. Doing this incrementally like that will just leave behind a big mess.

Also, what about marking the fields private to avoid the ABI mismatch?

@WalterBright
Copy link
Member Author

It's a common and reasonable approach to encapsulation when using virtual functions (I've even used it with D in object.d). Reducing the coupling from the front end to the back end is a worthwhile goal, and will decrease the brittleness of the .d/.h interface, not increase it.

As for the other compilers, isn't that what the Travis CI test does?

@ibuclaw
Copy link
Member

ibuclaw commented Aug 3, 2016

As for the other compilers, isn't that what the Travis CI test does?

The TravisCI tests only check that other compilers can build dmd. If we did use any of these fields, we wouldn't know until we next sync up our sources with yours.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 3, 2016

I have nothing to add to what concerns have already been said. However I will echo the importance of improving the quality of the interface for the back, I just think there are better ways to spend our time doing that.

@wilzbach
Copy link
Contributor

wilzbach commented Aug 3, 2016

The TravisCI tests only check that other compilers can build dmd. If we did use any of these fields, we wouldn't know until we next sync up our sources with yours.

That sounds like a good challenge for more automation! Is the increased build time the only thing that is blocking you from doing this or are you still too much out of sync? @klickverbot @JohanEngelen afaik is LDC now (nearly) in sync with DMD?

@JohanEngelen
Copy link
Contributor

@wilzbach LDC is pretty much in sync with the stable branch.
If you want to automate things, the usual steps are:

  1. checkout dmd commit of last sync
  2. copy LDC's ddmd dir over dmd's src and commit it
  3. merge dmd commit hash up to where you want to sync
  4. copy files in src and src/root back to LDC's ddmd dir.
  5. done!
    Merge conflicts aside, dmd's src dir contains a bunch of backend-specific files and other things that we don't use. Those should be ignored of course (moving the backend-specific files into backend would be nice for us). If files were added/removed, you need to determine whether they should be added/removed for LDC too.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 13, 2016

Is the increased build time the only thing that is blocking you from doing this or are you still too much out of sync?

I wrote a lengthy rant in the mailing list some time ago. In short travisci is a terrible platform because.

  • Kills build after 40 minutes (not enough time for single thread builds)
  • Kills build after excessing 4GB memory (so can't speed up using -j)
  • Kills build after 10 minutes silence in console (so must increase verbosity)
  • Only keeps the first thousand lines of console output.

I think I got build suceeding only once. But that required using some memory-saving linker switches, and by using parallel mutex locks to prevent two linkers running at once. But travisci has hardware that ranges from ok to terrible, so builds got killed more than they succeeded.

@kinke
Copy link
Contributor

kinke commented Aug 13, 2016

Kills build after 40 minutes (not enough time for single thread builds)

I can't confirm that. The Linux multilib job for LDC used to take slightly more than an hour, and that wasn't an issue. The OSX release job used to take almost 50 mins and also worked; I think the jobs got killed after 50 mins on OSX.

Kills build after excessing 4GB memory (so can't speed up using -j)

We use -j3 when building LDC, druntime & Phobos; -j2 for the Phobos unit tests, and have no issue with insufficient memory (on both Linux and OSX; the OSX VM is supposed to feature 3GB only).

Kills build after 10 minutes silence in console (so must increase verbosity)

True, but again no issue for LDC. What step would take that long?

Only keeps the first thousand lines of console output.

I recall some limitation, which could be worked around by displaying the raw log, which was always complete. A recent LDC job features almost 15k lines though, which are fully available: https://travis-ci.org/ldc-developers/ldc/jobs/151737315

Note that Travis provides different build environments with different features (memory, CPU cores, boot time etc.): https://docs.travis-ci.com/user/ci-environment. We use the container-based one for Linux.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 13, 2016

We use -j3 when building LDC, druntime & Phobos; -j2 for the Phobos unit tests, and have no issue with insufficient memory (on both Linux and OSX; the OSX VM is supposed to feature 3GB only).

The frontend itself is small, but whereas you link against a prebuilt compiler library, I have an entire compiler to build from source. ;-)

https://docs.travis-ci.com/user/common-build-problems/#My-build-script-is-killed-without-any-error

And it lists other problems I ran into also.

What step would take that long?

The D2 testsuite. Which is silent except for failures.

@WalterBright
Copy link
Member Author

Closed for lack of interest.

@ibuclaw
Copy link
Member

ibuclaw commented Mar 27, 2017

Also I am currently doing the opposite and adding fields and methods that you leave out of the headers (#6643)

@WalterBright
Copy link
Member Author

dmd no longer has any need for those .h files, so I leave decisions on how to deal with them up to you and the ldc team. I recommend, however, finding ways to reduce dependency on them.

@ibuclaw
Copy link
Member

ibuclaw commented Mar 29, 2017

Well my plan is to align up the C++ codebase to the stable branch just enough so I can begin testing bootstrapping the D frontend on gdc without having to maintain a separate copy.

Reducing dependence would entail another top-down rewrite. But not impossible, just wouldn't hold my breath for it to happen. :-)

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.

10 participants