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

[SR-4373] CheckResults in benchmarks always composes error message #46952

Closed
palimondo mannequin opened this issue Mar 27, 2017 · 4 comments
Closed

[SR-4373] CheckResults in benchmarks always composes error message #46952

palimondo mannequin opened this issue Mar 27, 2017 · 4 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. performance

Comments

@palimondo
Copy link
Mannequin

palimondo mannequin commented Mar 27, 2017

Previous ID SR-4373
Radar None
Original Reporter @palimondo
Type Bug
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Project Infrastructure
Labels Bug, Performance
Assignee @palimondo
Priority Medium

md5: 6c90aed7c80140d657ffe18e11de6125

Issue Description:

I was playing around with profiling benchmarks (Suffix.swift) and I’ve noticed that for large N, the tight loops were dominated by String interpolation instead of whatever the test was supposed to test. It looks like the CheckResults function always constructs the error message.

public func CheckResults(_ res: Bool, _ message: String = ""){
    if res {
        return
    }
    print(message)
    abort()
}

I guess we should redefine it with @autoclosure:

public func CheckResults(_ resultsMatch: Bool, _ message: @autoclosure () -> String){
    guard resultsMatch else {
        print(message())
        abort()
    }
}

I’ve taken the liberty to remove default value for message, all existing tests provide it and aborting without it is not very useful anyway.

@belkadan
Copy link
Contributor

Nice catch! The @autoclosure solution looks good to me, although I don't own the benchmarks. Can you send us a pull request if you haven't already?

@palimondo
Copy link
Mannequin Author

palimondo mannequin commented Mar 30, 2017

#8429

@palimondo
Copy link
Mannequin Author

palimondo mannequin commented Apr 5, 2017

Given this issue was with project infrastructure and my PR was merged to master (no need to wait for shipping release), I guess I’m good to close this (right?).

@belkadan
Copy link
Contributor

belkadan commented Apr 5, 2017

Yep, once it's merged it get moved to Resolved, and then once the originator is happy with it they can close it. That's you in both cases here, so feel free to close it.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. performance
Projects
None yet
Development

No branches or pull requests

1 participant