remove reference frontend.h source file#11910
remove reference frontend.h source file#11910WalterBright wants to merge 1 commit intodlang:masterfrom
Conversation
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#11910" |
WalterBright
force-pushed
the
no-frontend.h
branch
from
c22dde5 to
970020e
Compare
Where is this command coming from? Grepping for cxx-headers-test in the dmd repository turns up nothing. |
Line 499 in 9991401 |
This is probably the one you're looking for: Line 188 in 9991401
|
WalterBright
force-pushed
the
no-frontend.h
branch
from
970020e to
d8ffb8d
Compare
|
Thank you, @jacob-carlborg ! I had no idea grep ignored those. |
|
I agree that hidden files/directories are an abomination. Windows has them, too. |
There was a problem hiding this comment.
Well, since the introduction of frontend.h, there have only been two regressions in the C++ interface that got missed by those making changes on the D side (ignoring one stylistic change).
Before that, regressions were frequent, wasting hours of debugging time. I for one feel more relaxed now that other people are being informed that they need to fix the headers before merging their changes, though all the worrying has now been replaced with changes that expose the language implementation is fundamentally broken. That is a good thing, however. Fixing these issues pays off a lot more than fixing out-of-sync C++ headers. By not having the C++ header test means that I'll regress back to fixing hard to debug C++ header issues.
I don't understand this. Now people do not fix them, they just auto-generate them. If they are auto-generated instead of checked in, they will always be in sync. Just like the object files are always in sync with the code. |
WalterBright
dismissed
ibuclaw’s stale review
Review did not request changes.
|
I understand that, but the file does not need to be checked in to do that. Just have frontend.h generated into the |
WalterBright
dismissed
MoonlightSentinel’s stale review
I do not see the point of requesting changes when there are no changes requested.
Right, that is a crucial thing to mention. There are still some holes or problems with the dmd codebase that prevent use of the generated headers by the C++-based compilers. Even if all works well, there's still a bootstrap problem, so checking in the automated headers is here to stay for the time being. |
It is not necessary. As I remarked: "Just have frontend.h generated into the generated directory. Then, if you suspect a change, diff it with one from another build." There's no reason to check it in, just like there is no reason to check in any other generated file. |
The checked in header is used as a reference point for one build to another. Without it we're back to "gdc/ldc is broken, there have been 50 changes made over the last 2 weeks, and I have no idea whodunnit". |
Binary search. Besides, diff still works just as well as looking at github diff. |
Saying you disapprove of this PR is not a "changes requested". Besides, I did point out equivalent functionality - Also, I did not suggest removing generation of frontend.h. I understand that is useful. The PR still generates it. |
|
The point is to make it hard for a PR to break things unnoticed. |
It does not. It just makes my PRs constantly go stale and I, like everyone else, just generates them again. For example, the last change altered several thousand lines of frontend.h. Who vets that? Nobody. It's just regenerated and checked in. Generating frontend.h and dropping it in the "generated" directory accomplishes the same thing, but does not keep breaking PRs. |
That's why my comment offered a solution: Provide a replacement that doesn't rely on tracking |
I don't think everyone just generates them again, I would look for a way which does not break the interface. In any case something where manual re-generation is required is still more discouraging to breaking interface changes, than it just being automatically re-generated. |
It's even in the directions - if it's different, just regenerate it. How would that discourage anyone from making changes? |
By being another manual step. |
whenever deciding to update ldc/gdc. You pretty much have to do that anyway with the checked in one, it is not even extra work. Just different paths. Keep in mind I did not suggest not generating frontend.h. Only not checking it in. |
It that prevents a developer from making changes to a data structure, he shouldn't be working on the code. Seriously. |
|
I'm on the fence with this - On one hand, the experience when first dealing with That being said, pass the initial hurdleS, I do understand the uses for it. It did catch a few instances where I didn't notice a C++ ABI change was taking place (although they were also false positives...). Personally I'd be much happier if:
|
I agree but when the data-structure is on an interface boundary there is increased value in keeping it stable. |
It's wasting my time, and I'm tired of telling repeat offenders that their changes broke gdc due to their carelessness. With it being in the pipeline, that job is automated for me. |
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
No, it fulfills it purpose. It drastically reduces the number of C++ interface regressions and furthermore it will replace the hand-written headers soon.
|
I think the way forward is to auto diff it for the user. Humans shouldn't be doing what computers can do for them. |
|
Humans shouldn't be making work for other humans.
…On Tue, Oct 27, 2020, 7:46 PM Atila Neves ***@***.***> wrote:
I think the way forward is to auto diff it for the user. Humans shouldn't
be doing what computers can do for them.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11910 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVWSCDZCWDX4VRFDPTYBIDSM4ISDANCNFSM4TANXNVQ>
.
|
Correct. I don't know what the relevance of that comment is though. My point is that code should be doing most of the work. |
That's already the case. The file gets updated automatically and git/GitHub creates the diff. |
|
frontend.h, the file that never stops pointlessly breaking PRs. #11846 |
In defence of it, the list of dtoh changes will be somewhere close to 30 this release, most will have caused a significant change in the frontend.h file. In offence of it, I've seen no alternative proposal for ensuring that new PRs both fail the pipeline when a C++ header isn't updated, and alert reviewers to take a closer look at the new definitions added to D and C++ sources. |
You can take a note of the front end git version last time you built a gdc. When building a new one against the latest front end, diff the generated frontend.h against the one from the last time. How often do you port to the latest master? This is surely much less work than breaking PRs every time a declaration changes, each one requiring manual work by the submitter. |
|
In fact, that's pretty much what you have to do regardless. You're still going to have to remember which version was the last one you used, and diff from the current to that one. |
That's not taking a proactive approach to ensure the interface is never broken.
Less often than I'd like to.
Conservatively, each broken dmd implementation can waste up to three days going on a week if it is silent corruption due to interface mismatch - rebase gcc; sync dmd; clean; bootstrap; regression test; debug; determine which "end" might be at fault; fix; rebase gcc; sync dmd; clean; bootstrap; regression test; merge fix with upstream dmd; write cxxfrontend test to ensure that part doesn't break again; raise PR; rebase gcc; sync dmd after merge of PR; clean; bootstrap; regression test; write changelog; rebase gcc; commit to gcc (check results of arm/sparc/ppc/... builds next day for druntime/phobos regressions). |
|
It has occurred to me that when the frontend.h header gets to a point of being include-able, and the hand maintained headers go away, we'll need to check in the generated header into dmd anyway, as it will be the source of truth for downstream merges with gdc and ldc. Otherwise, if we do it the way you suggest and keep the generated header checked in downstream only, it'll be immediately out of sync after a merge with upstream dmd, and there's no way to generate a new header as the bootstrap compiler won't work. Not without adding more hoops to jump through to do what should be a simple three step process (merge, compile and test) to update the D frontend. |
|
It has also occurred to me that the header generated by dmd may not be correct for gdc or ldc anyway - gdc because there are differences in back-end types unless you compile with |
|
I don't mind the rebase, but I mind it being hard to update. Personally, I still have to do it mostly by hand. CC @MoonlightSentinel . |
|
Note that one thing that will never be solved unless we eliminate it (which luckily @WalterBright is in the process of doing) is this: enum class TARGET : bool
{
- Linux = true,
- OSX = false,
+ Linux = false,
+ OSX = true,
FreeBSD = false,
OpenBSD = false,
Solaris = false, |
Yep, it should be private to dmd.target. |
I wrote down an informal proposal, though someone else should do the work as you'll be holding your breath for a long time if you expect me to find the time to do such a thing. :-) https://issues.dlang.org/show_bug.cgi?id=21524 Cc @kinke naturally your input matters more so than mine, so please slap me round the head if any kind of middle ground suggestion is nonsense. |
This can propably be improved a lot by
The latter quite hard because the generated AST changes w.r.t. the current target settings (this definitly needs to be improved for DMD as a library) |
Wut? ;) - I'm not too keen on having an extra layer for us C++ users. IMO, the situation wasn't that bad before I haven't really looked at |
Well they are already |
|
Giving that:
I suggest that we bare with @WalterBright are you still not convinced? I suggest that we close this. |
|
Since there were no objections, I will take the liberty of closing this. Please reopen at any time if you do not agree with my assessment. |
11 participants
The reason given for having frontend.h as a reference file is "to see if it changes". But the advice when it changes is "just run build to auto-generate it". Which means that it is pointless and a nuisance to have a reference file, for the same reason it is pointless and a nuisance to check in the generated object files.
frontend.h egregiously violates the DRY principle, and has wasted a great deal of my time updating it when thousands of lines of it randomly change, as just happened to my PRs.