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

Extract include func to dsymbolsem #16970

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

dchidindu5
Copy link
Contributor

@dchidindu5 dchidindu5 commented Oct 7, 2024

public bool addisdone = false; /// true if members have been added to scope
public bool onStack = false;

This variables were private before I had to change it to public because I encountered errors like undefined identifier onStack and addisdone`

Still working on them though

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dchidindu5! 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 run digger -- build "master + dmd#16970"

compiler/src/dmd/attrib.d Show resolved Hide resolved
compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved
compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved
compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved
compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved
private bool addisdone = false; /// true if members have been added to scope
private bool onStack = false; /// true if a call to `include` is currently active
public bool addisdone = false; /// true if members have been added to scope
public bool onStack = false; /// true if a call to `include` is currently active
Copy link
Contributor

Choose a reason for hiding this comment

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

also be sure to update attrib.h and frontend.h to reflect the changes made

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd do that once I'm done with the refactoring

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved
compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved
{
alias visit = typeof(super).visit;
Scope* sc;
Dsymbols* symbols;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with this declaration? This shouldn't be here. AttribDeclaration defines the include method and all the children of AttribDeclaration override this method hence they have access to the array of declarations. I assume you actually wanted to have access to the decl field of AttribDeclaration? If you, you need to add a visitation method for AttribDeclaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't totally understand this please

@dchidindu5
Copy link
Contributor Author

can it be casted?
e.gLine 7539- cast(void) cache;

what about return null ?
e.g line 7535
Can it be removed entirely or to be casted?
@thewilsonator

{
assert(!sfd.onStack);
symbols= sfd.cache;
}
Copy link
Contributor

@RazvanN7 RazvanN7 Oct 10, 2024

Choose a reason for hiding this comment

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

This is equivalent to:

if (sfd.errors || sfd.onStack)
{
        if (sfd.cached)
        {
            assert(!sfd.onStack);
            symbols= sfd.cache;
        }
}

Whereas what you want is:

if (sfd.errors || sfd.onStack)
{
      symbols = null;
      return;
}
if (sfd.cached)
{
       assert(!sfd.onStack);
       symbols= sfd.cache;
       return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also symbols = sfd.cache; with proper spaces.

@dchidindu5
Copy link
Contributor Author

What's the appropriate way to cast this
return condition.include(cdc._scope ? cdc._scope : sc) ? decl : cdc.elsedecl;

this(Scope* sc)
{
this.sc = sc;
this.symbols = symbols;
Copy link
Contributor

Choose a reason for hiding this comment

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

This assignment doesn't make any sense. Delete it.

}
assert(cdc.condition);
symbols = cdc.condition.include(cdc._scope ? cdc._scope : sc) ? cdc.decl : cdc.elsedecl;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This return is unnecessary.

sfd.cached = true;
sfd.cache = d;
symbols = d;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This return is not necessary.

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

Successfully merging this pull request may close these issues.

4 participants