-
Notifications
You must be signed in to change notification settings - Fork 612
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
profile.CheckValid should return an error if location has a line with empty function #517
Conversation
Codecov Report
@@ Coverage Diff @@
## master #517 +/- ##
=======================================
Coverage 67.06% 67.06%
=======================================
Files 37 37
Lines 7611 7613 +2
=======================================
+ Hits 5104 5106 +2
Misses 2103 2103
Partials 404 404
Continue to review full report at Codecov.
|
profile/profile.go
Outdated
@@ -402,6 +402,8 @@ func (p *Profile) CheckValid() error { | |||
if f.ID == 0 || functions[f.ID] != f { | |||
return fmt.Errorf("inconsistent function %p: %d", f, f.ID) | |||
} | |||
} else { | |||
return fmt.Errorf("profile has nil function in location line") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we include any context in the error message that will help triage the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
profile/profile.go
Outdated
@@ -402,6 +402,8 @@ func (p *Profile) CheckValid() error { | |||
if f.ID == 0 || functions[f.ID] != f { | |||
return fmt.Errorf("inconsistent function %p: %d", f, f.ID) | |||
} | |||
} else { | |||
return fmt.Errorf("profile has nil function in location line") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the nil check first and unindent the main path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please clarify the title of the PR. "Check" is insufficient, document what the outcome is.
Bump from 20191205061153 => 20201109224723 My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`) Includes: - google/pprof#564 - google/pprof#575 - google/pprof#574 - google/pprof#571 - google/pprof#572 - google/pprof#570 - google/pprof#562 - google/pprof#561 - google/pprof#565 - google/pprof#560 - google/pprof#563 - google/pprof#557 - google/pprof#554 - google/pprof#552 - google/pprof#545 - google/pprof#549 - google/pprof#547 - google/pprof#541 - google/pprof#534 - google/pprof#542 - google/pprof#535 - google/pprof#531 - google/pprof#530 - google/pprof#528 - google/pprof#522 - google/pprof#525 - google/pprof#527 - google/pprof#519 - google/pprof#520 - google/pprof#517 - google/pprof#518 - google/pprof#514 - google/pprof#513 - google/pprof#510 - google/pprof#508 - google/pprof#506 - google/pprof#509 - google/pprof#504
* Update pprof to latest revision Bump from 20191205061153 => 20201109224723 My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`) Includes: - google/pprof#564 - google/pprof#575 - google/pprof#574 - google/pprof#571 - google/pprof#572 - google/pprof#570 - google/pprof#562 - google/pprof#561 - google/pprof#565 - google/pprof#560 - google/pprof#563 - google/pprof#557 - google/pprof#554 - google/pprof#552 - google/pprof#545 - google/pprof#549 - google/pprof#547 - google/pprof#541 - google/pprof#534 - google/pprof#542 - google/pprof#535 - google/pprof#531 - google/pprof#530 - google/pprof#528 - google/pprof#522 - google/pprof#525 - google/pprof#527 - google/pprof#519 - google/pprof#520 - google/pprof#517 - google/pprof#518 - google/pprof#514 - google/pprof#513 - google/pprof#510 - google/pprof#508 - google/pprof#506 - google/pprof#509 - google/pprof#504 * Update P/pprof/build_tarballs.jl - use a real version number Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> * Remove now unused `timestamp` * [pprof] Use `GitSource` Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
… empty function (google#517) * Check if location has a line with empty function * Include context in the error message that will help triage the error
CheckValid should check that all lines in locations have non nil function pointer.