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

Make Type hierarchy methods const #10319

Closed
wants to merge 1 commit into from

Conversation

edi33416
Copy link
Contributor

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

For a detailed description of the motivation, please see PR

@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#10319"

@edi33416
Copy link
Contributor Author

@rainers
Copy link
Member

rainers commented Aug 19, 2019

The number of necessary cast() operations is a bit worrying. Maybe this can be mitigated by declaring methods like nextOf, memType and elementType inout?

@jacob-carlborg
Copy link
Contributor

I suggest you start with the leaf functions to avoid the need for cast() all over the place. Unfortunately it seems that DMD modifies the internal state of AST nodes in a lot of the getXXX and isXXX methods.

{
return target.alignsize(this);
return target.alignsize(cast() this);
Copy link
Member

Choose a reason for hiding this comment

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

You should update Target::alignsize as receiving a const parameter to avoid unnecessary cast.

{
return basetype.size();
return (cast() basetype).size();
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary? You're not making modifications here.

@edi33416
Copy link
Contributor Author

The number of necessary cast() operations is a bit worrying.

I agree. Especially since, as @jacob-carlborg pointed out:

Unfortunately it seems that DMD modifies the internal state of AST nodes in a lot of the getXXX and isXXX methods.

Maybe this can be mitigated by declaring methods like nextOf, memType and elementType inout?

I'll give inout a try. I attempted to make them const and that just blew up in my face.

@jacob-carlborg
Copy link
Contributor

inout will not help with the get and is methods. An inout will behave inside the method as a const method.

const in D does not support logical const. It’s been mentioned thousands of times.

@jacob-carlborg
Copy link
Contributor

You can try my tool DLP to identify leaf functions. https://github.com/jacob-carlborg/dlp

@edi33416
Copy link
Contributor Author

edi33416 commented Aug 21, 2019

I think that because the get and is methods lazily initialise internal state, properly adding const throughout the code base will require a lot of non trivial changes. I propose that we delete the const qualifier from the few methods that have one, in the Type hierarchy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants