Skip to content

Make RootObject equals and toChars const#10293

Merged
thewilsonator merged 5 commits intodlang:masterfrom
edi33416:rootobj_covariance
Aug 20, 2019
Merged

Make RootObject equals and toChars const#10293
thewilsonator merged 5 commits intodlang:masterfrom
edi33416:rootobj_covariance

Conversation

@edi33416
Copy link
Contributor

@edi33416 edi33416 commented Aug 9, 2019

This PR is useful to the the C++ header generator to generate the dmd frontend header files that are used by gdc.

The issue that I'm attempting to solve: D has function covariance, but C++ does not. What this means is that if I have the following D code

extern (C++) class A
{
  void foo() {}
}

extern (C++) class B :  A
{
  override void foo() const {}
}

the generated header will look like

class A
{
  virtual void foo();
}

class B : public A
{
  void foo() const;
}

Note that B has void foo() const
Compiling this code with g++ -Woverloaded-virtual (as gdc does) will result in the following warning

warning: 'virtual void A::foo()' was hidden

by `void B::foo() const`

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10293"

@edi33416
Copy link
Contributor Author

edi33416 commented Aug 9, 2019

@andralex

@ibuclaw
Copy link
Member

ibuclaw commented Aug 9, 2019

Not all sources look to be updated, headers should be updated as well too if they haven't been.

@edi33416
Copy link
Contributor Author

edi33416 commented Aug 9, 2019

Nothing all sources look to be updated,

I'm sorry, but what do you mean by this?

headers should be updated as well too if they haven't been.

I haven't updated the headers as they have dropped the const from the function declaration in order to work. Adding the const won't change the name mangling so I left the headers unchanged, in the "hope" of getting the auto-generated header in soon. I've managed to build gdc's decl.cc with the auto-generated header, so I really hope we'll get there soon.

@edi33416 edi33416 requested a review from ibuclaw as a code owner August 9, 2019 22:04
@ibuclaw
Copy link
Member

ibuclaw commented Aug 10, 2019

I'm sorry, but what do you mean by this?

I think I meant not, but autocomplete got in the way. There were some D related errors at last look.

Copy link
Member

@rainers rainers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

@rainers
Copy link
Member

rainers commented Aug 10, 2019

Adding the const won't change the name mangling

It does on Windows and I would be surprised if it doesn't elsewhere (e.g. for overloads), but it is usually irrelevant for virtual function calls if the vtbl is generated on the D side.

@edi33416
Copy link
Contributor Author

@rainers do you know why the tests are failing? I didn't manage to find the struct S that's causing problems with toString. I guess that struct needs to override with a toString() const.

@Geod24
Copy link
Member

Geod24 commented Aug 19, 2019

@edi33416 :

string toString()

@edi33416
Copy link
Contributor Author

@edi33416 :

string toString()

I hadn't rebase with master..

@edi33416 edi33416 force-pushed the rootobj_covariance branch from a9fb413 to bd34cc5 Compare August 19, 2019 14:51
@rainers
Copy link
Member

rainers commented Aug 19, 2019

LGTM. Please squash commits. (Or do we use "squash and merge"? I suspect it will just concatenate commit messages that don't look to nice).

@thewilsonator
Copy link
Contributor

I'll just do the squashing manually.

@thewilsonator thewilsonator merged commit e01fa73 into dlang:master Aug 20, 2019
@Geod24
Copy link
Member

Geod24 commented Aug 20, 2019

@rainers : FYI, Using squash and merge on Github allows you to edit the message. But the bot just squash them, yes.

dkorpel pushed a commit to dkorpel/dmd that referenced this pull request Aug 20, 2019
@edi33416
Copy link
Contributor Author

LGTM. Please squash commits. (Or do we use "squash and merge"? I suspect it will just concatenate commit messages that don't look to nice).

I thought the squash and merge will keep only the first commit message. That's why I didn't do it myself and why I just named the future commits "qf".. Will pay more attention in the future. Thanks @thewilsonator for doing it for me

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