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.

Sign up for GitHub

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

Jump to bottom

Fix issue 18097 - unittest symbol names can be used before semantic #7761

Merged
merged 2 commits into from
Feb 1, 2018
Merged

Fix issue 18097 - unittest symbol names can be used before semantic #7761

merged 2 commits into from
Feb 1, 2018

Conversation

atilaneves
Copy link
Contributor

@atilaneves atilaneves commented Jan 22, 2018 •
edited
Loading

Not only does this PR fix issue 18097, it also simplifies the unittest identifier logic and fixes the test for related issue 16995.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 22, 2018 •
edited
Loading

Thanks for your pull request, @atilaneves! 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
✗ 16995 __traits(getUnittests) doesn't work with separate compilation
✓ 18097 [REG2.077] Unittest function is undefined identifier

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

@atilaneves atilaneves force-pushed the fix_18097 branch 2 times, most recently from 6ab7fc8 to 70aa934 Compare January 22, 2018 17:16
@JinShil
Copy link
Contributor

JinShil commented Jan 22, 2018

Let me know when this is no longer a WIP

@JinShil JinShil added the Review:WIP Work In Progress - not ready for review or pulling label Jan 22, 2018
@atilaneves atilaneves changed the title WIP: Fix issue 18097 - unittest symbol names can be used before semantic Fix issue 18097 - unittest symbol names can be used before semantic Jan 23, 2018
@atilaneves
Copy link
Contributor Author

@JinShil No longer WIP and ready for review.

@JinShil JinShil removed the Review:WIP Work In Progress - not ready for review or pulling label Jan 24, 2018
wilzbach
wilzbach reviewed Jan 25, 2018
View reviewed changes
test/runnable/issue16995.d Outdated
shared static this()
{
import core.runtime: Runtime;
Runtime.moduleUnitTester = () => true;
Copy link
Member

Choose a reason for hiding this comment

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

Returning bool is deprecated, return a number instead.
See also: dlang/druntime#1685

@atilaneves
Copy link
Contributor Author

@wilzbach Return UnitTestResult then, not an int. Changed.

@jacob-carlborg jacob-carlborg mentioned this pull request Jan 25, 2018
Merged
ibuclaw
ibuclaw reviewed Feb 1, 2018
View reviewed changes
src/dmd/func.d
properly in the semantic pass. See:
https://issues.dlang.org/show_bug.cgi?id=16995
*/
final void setIdentifier()
Copy link
Member

Choose a reason for hiding this comment

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

Was this added to headers also?

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 grepped and didn't find it anywhere.

@dlang-bot dlang-bot merged commit e2b75ef into dlang:master Feb 1, 2018
@atilaneves atilaneves deleted the fix_18097 branch February 5, 2018 16:32
JohanEngelen added a commit to weka/ldc that referenced this pull request Mar 7, 2018
@JohanEngelen
Cherry-pick from DMD upstream e2b75ef17e2640ea2123243b82491d11c07a9848
d0b9bf0 
PR dlang/dmd#7761

Fixes unittest symbol resolution bug https://issues.dlang.org/show_bug.cgi?id=18097.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilzbach wilzbach wilzbach left review comments

@ibuclaw ibuclaw ibuclaw approved these changes

@JinShil JinShil JinShil approved these changes

@dnadlinger dnadlinger Awaiting requested review from dnadlinger

Assignees
No one assigned
Labels
Merge:auto-merge Severity:Bug Fix Severity:Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@atilaneves @dlang-bot @JinShil @ibuclaw @wilzbach