Skip to content

Comments

Make expression.d methods private where possible#7251

Merged
MartinNowak merged 13 commits intodlang:masterfrom
RazvanN7:Expression_private_methods
Oct 27, 2017
Merged

Make expression.d methods private where possible#7251
MartinNowak merged 13 commits intodlang:masterfrom
RazvanN7:Expression_private_methods

Conversation

@RazvanN7
Copy link
Contributor

This PR applies the private keyword to methods in expression.d where possible. In some cases it moves functions to other files so that private can be applied.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

Looks good, apart from a couple of minor comments.

Nice work on splitting the refactoring in different commits. (The extra changes that I requested could be done via git rebase -i + git commit --amend, to keep clean history of your branch.)

return result;
}

/***********************************************
Copy link
Member

Choose a reason for hiding this comment

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

Didn't you mean to move modifyFieldVar to src/ddmd/expressionsem.d?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the function is called just once from declaration.d. Don't know why it was in expression.

Copy link
Member

Choose a reason for hiding this comment

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

I reviewed the pull-request commit by commit and I noticed that the commit message of this change was "Move mofifyFieldVar to expressionsem and make it private", so it would be better to change the commit message to "Move mofifyFieldVar to declaration and make it private", in order to avoid confusion.

return new ErrorExp();
return e;
}

Copy link
Member

Choose a reason for hiding this comment

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

Nit: remove extra whitespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What extra whitespace?

Copy link
Member

@PetarKirov PetarKirov Oct 27, 2017

Choose a reason for hiding this comment

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

Never mind, there were three empty lines in 320581b#diff-b90dc1ef3092d86c410bd0a78271e80eR335, but it looks like you fixed this a later commit.

* Returns:
* true a semantic error was detected
*/
private extern (C++) bool arrayExpressionToCommonType(Scope* sc, Expressions* exps, Type* pt)
Copy link
Member

Choose a reason for hiding this comment

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

Remove extern (C++) - it's clearly not needed as arrayExpressionToCommonType can only be called by D code from this module.

/***************************************
* Pull out any properties.
*/
private extern (C++) Expression resolvePropertiesX(Scope* sc, Expression e1, Expression e2 = null)
Copy link
Member

Choose a reason for hiding this comment

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

Remove extern (C++).

* This doesn't appear to do anything.
*/
extern (C++) bool checkPropertyCall(Expression e, Expression emsg)
private extern (C++) bool checkPropertyCall(Expression e, Expression emsg)
Copy link
Member

Choose a reason for hiding this comment

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

Remove extern (C++).

* Returns:
* true an error was issued
*/
private extern (C++) bool checkDefCtor(Loc loc, Type t)
Copy link
Member

Choose a reason for hiding this comment

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

Remove extern (C++).

* Mark variable v as modified if it is inside a constructor that var
* is a field in.
*/
private extern (C++) int modifyFieldVar(Loc loc, Scope* sc, VarDeclaration var, Expression e1)
Copy link
Member

Choose a reason for hiding this comment

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

Remove extern (C++).

}

extern (C++) Expression opAssignToOp(Loc loc, TOK op, Expression e1, Expression e2)
private extern (C++) Expression opAssignToOp(Loc loc, TOK op, Expression e1, Expression e2)
Copy link
Member

Choose a reason for hiding this comment

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

Remove extern (C++).

/****************************************************************/

extern (C++) Expression extractOpDollarSideEffect(Scope* sc, UnaExp ue)
private extern (C++) Expression extractOpDollarSideEffect(Scope* sc, UnaExp ue)
Copy link
Member

Choose a reason for hiding this comment

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

Remove extern (C++).

return (err || olderrors != global.errors);
}

private extern (C++) Module loadStdMath()
Copy link
Member

Choose a reason for hiding this comment

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

Remove extern (C++).

@PetarKirov
Copy link
Member

PetarKirov commented Oct 27, 2017

The Jenkins failure should be fixed by dlang/phobos#5816.

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

noice
cc @WalterBright

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

LGTM

@MartinNowak
Copy link
Member

Looking forward to resolve merge conflicts ;).

@MartinNowak MartinNowak merged commit d60fbe8 into dlang:master Oct 27, 2017
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.

5 participants