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

Fix: command_line review items & Coverity lints #69

Merged
merged 19 commits into from
Jun 13, 2023

Conversation

dragonmux
Copy link
Member

This PR addresses the review items bought up when code reviewing <substrate/command_line> with @freyjadomville. It also addresses several items found when running a Coverity scan such as missed handling of std::variant::valueless_by_exception() == true, resulting in possible exception throwing.

Additionally, by request, this addresses some exception mishaps in affinity_t's tests too (again, found by Coverity). As we addressed those, we also cast the net a little broader and went after lints in other parts of the library to help reduce the Coverity footprint and improve the code quality.

@dragonmux dragonmux added the bug Something isn't working label Jun 6, 2023
@dragonmux dragonmux requested a review from amyspark June 6, 2023 05:40
@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #69 (ae3b4e0) into main (c1a241c) will decrease coverage by 0.61%.
The diff coverage is 70.12%.

@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
- Coverage   92.62%   92.01%   -0.61%     
==========================================
  Files          47       47              
  Lines        3175     3208      +33     
  Branches      609      626      +17     
==========================================
+ Hits         2941     2952      +11     
- Misses        177      188      +11     
- Partials       57       68      +11     
Impacted Files Coverage Δ
substrate/command_line/options 89.09% <0.00%> (-3.37%) ⬇️
substrate/hash 94.16% <ø> (ø)
impl/command_line/options.cxx 85.71% <25.00%> (-5.01%) ⬇️
impl/command_line/arguments.cxx 85.49% <55.88%> (-4.71%) ⬇️
substrate/enum_utils 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@amyspark amyspark left a comment

Choose a reason for hiding this comment

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

Some nitpicks here and there, the one most bothersome is the string v. string view issue and the Qt-style implicit copy when iterating over map_t.

impl/command_line/options.cxx Show resolved Hide resolved
substrate/enum_utils Show resolved Hide resolved
substrate/enum_utils Outdated Show resolved Hide resolved
substrate/enum_utils Outdated Show resolved Hide resolved
substrate/enum_utils Outdated Show resolved Hide resolved
test/affinity.cxx Outdated Show resolved Hide resolved
test/affinity.cxx Outdated Show resolved Hide resolved
@amyspark
Copy link
Collaborator

amyspark commented Jun 6, 2023

@dragonmux One more comment which I forgot to add: is there a way to force a test for those valueless_by_exception() variant checks?

@dragonmux
Copy link
Member Author

Pushed fixes for the various review items - thank you Amy!

We've been contemplating the valueless_by_exception() problem and haven't yet thought of something - unfortunately.

@dragonmux dragonmux force-pushed the fix/command-line-review-items branch 2 times, most recently from 1877b9f to 6b5290f Compare June 7, 2023 12:52
@dragonmux dragonmux requested a review from amyspark June 10, 2023 08:12
@dragonmux
Copy link
Member Author

We'll rebase this and get this merged then, thank you 🙂

…lueless_by_exception() making its way into expressions it shouldn't
…a check for std::variant::valueless_by_exception()
… check for std::variant::valueless_by_exception()
@dragonmux dragonmux force-pushed the fix/command-line-review-items branch from 6b5290f to ae3b4e0 Compare June 13, 2023 13:06
@dragonmux dragonmux merged commit ae3b4e0 into main Jun 13, 2023
162 of 165 checks passed
@dragonmux dragonmux deleted the fix/command-line-review-items branch June 13, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants