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

Improve sanity checking and error messages in PerfAnalyzer CLI #371

Merged
merged 11 commits into from
Aug 10, 2023
Merged

Improve sanity checking and error messages in PerfAnalyzer CLI #371

merged 11 commits into from
Aug 10, 2023

Conversation

nv-hwoo
Copy link
Contributor

@nv-hwoo nv-hwoo commented Jul 28, 2023 •
edited
Loading

Checks made on the inputs are:

  • Protect unsigned parameters against negative inputs (and potential overflow)
  • Validate file path
  • Validate non-negative integer range inputs (e.g. start:end)
  • Check for valid type/option
  • Rephrase all error messages to include the flag name for better clarity

@matthewkotila
Copy link
Contributor

Are these sanity checks driving anyone else crazy??

debermudez
debermudez reviewed Aug 1, 2023
View reviewed changes
Copy link
Contributor

@debermudez debermudez left a comment

Choose a reason for hiding this comment

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

Looks solid. Only really worried about the option name change

@nv-hwoo nv-hwoo mentioned this pull request Aug 4, 2023
Merged
@nv-hwoo nv-hwoo changed the title Add sanity checks against invalid PerfAnalyzer command line inputs Improve sanity checking and error messages in PerfAnalyzer CLI Aug 4, 2023
@matthewkotila
Copy link
Contributor

@nv-hwoo this PR is approved--was there something its waiting on before merging?

@nv-hwoo
Copy link
Contributor Author

nv-hwoo commented Aug 10, 2023

@matthewkotila Not that I know of. I will go ahead and merge this branch.

@nv-hwoo nv-hwoo merged commit 7a7cd2a into main Aug 10, 2023
3 checks passed
@nv-hwoo nv-hwoo deleted the hwoo-validate-cli-options branch August 10, 2023 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgerdesnv tgerdesnv tgerdesnv left review comments

@matthewkotila matthewkotila matthewkotila approved these changes

@debermudez debermudez debermudez approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nv-hwoo @matthewkotila @debermudez @tgerdesnv