Skip to content

Adapt to llvm NamedRegionTimer changes #6003

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

Closed
wants to merge 1 commit into from
Closed

Conversation

MatzeB
Copy link

@MatzeB MatzeB commented Dec 1, 2016

llvm requires a Name/Description pair for NamedRegionTimers as of
r287369. Because most of the previously used "names" for timers are
actually longer descriptions containing multiple words and spaces. I
added a Name parameter to the llvm side to produce machine parseable
output with names that rather resemble a programming language
identifier rather than a sentence.

This is the easiest adaption fix for the API change that just uses the existing string for
name+description of the timer.
It would be good if someone familiar with swift could add proper short
names to the timers.

llvm requires a Name/Description pair for NamedRegionTimers as of
r287369. Because most of the previously used "names" for timers are
actually longer descriptions containing multiple words and spaces. I
added a Name parameter to the llvm side to produce machine parseable
output with names that rather resemble a programming a language
identifier rather than a sentence.

This is the easiest possible fix that uses the existing string for
name+description of the timer just to be compatible with the new API.
It would be good if someone familiar with swift could add proper short
names to the timers.
@MatzeB
Copy link
Author

MatzeB commented Dec 1, 2016

apple/swift-llvm#35

@swift-ci Please smoke test

@CodaFi
Copy link
Contributor

CodaFi commented Dec 1, 2016

It looks like @compnerd beat you to the punch here in #5890.

@MatzeB
Copy link
Author

MatzeB commented Dec 1, 2016

Yep and his fix is actually complete. I hope I calmed down the build for now by adding compatibility constructors to the llvm side. So I can look at the swift side (and probably abandon this) calmly tomorrow.

@gottesmm
Copy link
Contributor

gottesmm commented Dec 2, 2016

@MatzeB Can this be closed?

@MatzeB
Copy link
Author

MatzeB commented Dec 2, 2016

Yep, let's close it in favor of #5890

@MatzeB MatzeB closed this Dec 2, 2016
@gottesmm
Copy link
Contributor

gottesmm commented Dec 2, 2016

Thanks!

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.

3 participants