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

fix Issue 17814 - bad output of "static foreach" with -vcg-ast #7126

Merged
merged 2 commits into from
Sep 16, 2017

Conversation

tgehr
Copy link
Contributor

@tgehr tgehr commented Sep 7, 2017

This implements PrettyPrintVisitor for StaticForeachDeclaration.

I'm not sure about the -vgc-ast test (I've just added new code). Is the output checked?

@dlang-bot
Copy link
Contributor

dlang-bot commented Sep 7, 2017

Thanks for your pull request, @tgehr! 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

Auto-close Bugzilla Description
17814 bad output of "static foreach" with -vcg-ast

@tgehr tgehr changed the base branch from master to stable September 7, 2017 20:24
@tgehr tgehr changed the base branch from stable to master September 7, 2017 20:24
@tgehr tgehr changed the base branch from master to stable September 7, 2017 20:28
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.

LGTM. Can the travis failure be ignored? It seems it tried to build against master instead of stable.

@stefan-koch-sociomantic
Copy link

@tgehr the output is not checked since -vcg-ast cannot really guarantee a stable output.

@stefan-koch-sociomantic
Copy link

@tgehr I do not agree with this fix though.
-vcg-ast is supposed to show the expanded versions of everything.
the static foreach should not even be in the output.

The fact that it is worries me.

@tgehr
Copy link
Contributor Author

tgehr commented Sep 8, 2017

@stefan-koch-sociomantic That's not related to static foreach. static if is not expanded at module scope either.

@stefan-koch-sociomantic
Copy link

stefan-koch-sociomantic commented Sep 8, 2017

ah, that's pretty bad.
I'll have a look later.
@tgehr

@WalterBright
Copy link
Member

There's lots of red in the code - meaning it isn't tested. Are you using the browser plugin that shows which lines are executed by the test suite?

@tgehr
Copy link
Contributor Author

tgehr commented Sep 10, 2017

@WalterBright: I usually just add tests that exercise all new functionality. My commit actually increases the total code coverage slightly (because PrettyPrintVisitor.visit(ForeachStatement) had not yet been covered). However, it is not possible to cover some of the code using external code with the -vcg-ast switch: static foreach over a range is lowered to static foreach over AliasSeq and static foreach statements are unrolled before -vcg-ast prints them.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

@WalterBright FWIW this even increases the overall project coverage...

@WalterBright
Copy link
Member

@tgehr is it possible to execute the code without the -vcg-ast switch being thrown?

@tgehr
Copy link
Contributor Author

tgehr commented Sep 16, 2017

@WalterBright: The bug report was about -vcg-ast, -vcg-ast uses the PrettyPrintVisitor to format source code. (I.e. -vcg-ast should not decrease PrettyPrintVisitor coverage.) I don't know DMD well enough to judge whether there is any other way to cover the parts of the PrettyPrintVisitor relevant for this pull request (but a lot of code in it is currently uncovered). My best guess is that there is no such way (i.e., some of the code in the PrettyPrintVisitor is probably dead code except when someone is printing some AST information during debugging).

@WalterBright WalterBright merged commit 591c7bb into dlang:stable Sep 16, 2017
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.

6 participants