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

[Refactoring] around protection code fixed in the PR #3651 #3906

Merged
merged 4 commits into from
Aug 28, 2014

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Aug 27, 2014

Tweak code style, slim up code logic, and a trivial bug fix.

@Dicebot I found two questions while refactoring.

  1. How __traite(getProtection) should print the new protection package(names)?
  2. How should the JSON property protection print package(names)?

@9rnsr 9rnsr force-pushed the refactor_protection branch from 8673c51 to 1b9dc9f Compare August 27, 2014 12:47
@mihails-strasuns
Copy link

Is there any reason why formatting used by https://github.com/D-Programming-Language/dmd/blob/master/src/attrib.c#L617 is not suitable?

@mihails-strasuns
Copy link

Though now that I think about it __traits should be kept to return plain "package" string even if explicit identifiers are present for backwards compatibility reasons. I don't know how much of a concern it is for JSON output.

@9rnsr
Copy link
Contributor Author

9rnsr commented Aug 27, 2014

Is there any reason why formatting used by https://github.com/D-Programming-Language/dmd/blob/master/src/attrib.c#L617 is not suitable?

Unused parameter name might be warned when dmd code is compiled by non-dmc compiler with extra warning options.

@9rnsr
Copy link
Contributor Author

9rnsr commented Aug 27, 2014

Though now that I think about it __traits should be kept to return plain "package" string even if explicit identifiers are present for backwards compatibility reasons.

Make sense.

I don't know how much of a concern it is for JSON output.

For backward compatibility, keeping protection property may be better. And for the new package(names), we can add one more JSON property, eg. from package(foo.bar.baz):

   protection: "package",
   protecionPackage: "foo.bar.baz"  // new

@mihails-strasuns
Copy link

For backward compatibility, keeping protection property may be better. And for the new package(names), we can add one more JSON property, eg. from package(foo.bar.baz)

Sounds good.

@9rnsr
Copy link
Contributor Author

9rnsr commented Aug 27, 2014

I filed the issue in bugzilla: https://issues.dlang.org/show_bug.cgi?id=13384

@9rnsr 9rnsr force-pushed the refactor_protection branch from 1b9dc9f to c0f0d0b Compare August 28, 2014 13:52
@9rnsr
Copy link
Contributor Author

9rnsr commented Aug 28, 2014

I resolved conflicts. Ready to merge.

@mihails-strasuns
Copy link

LGTM but no dmd merge access :)

@9rnsr
Copy link
Contributor Author

9rnsr commented Aug 28, 2014

@Dicebot Thanks for your LGTM.

@9rnsr
Copy link
Contributor Author

9rnsr commented Aug 28, 2014

Auto-merge toggled on

9rnsr added a commit that referenced this pull request Aug 28, 2014
[Refactoring] around protection code fixed in the PR #3651
@9rnsr 9rnsr merged commit 1f9abd5 into dlang:master Aug 28, 2014
@9rnsr 9rnsr deleted the refactor_protection branch August 28, 2014 16:36
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.

2 participants