Skip to content

Fix for GCC 8 warning#201

Merged
henryiii merged 2 commits intomasterfrom
henryiii-gcc8
Jan 25, 2019
Merged

Fix for GCC 8 warning#201
henryiii merged 2 commits intomasterfrom
henryiii-gcc8

Conversation

@henryiii
Copy link
Copy Markdown
Collaborator

@henryiii henryiii commented Jan 24, 2019 •
edited
Loading

This seems to be a bug in 1.7.0 introduced in #187; the == evaluates before the ?. GCC notices this and gives a warning.

@henryiii
Copy link
Copy Markdown
Collaborator Author

henryiii commented Jan 24, 2019

@phlptp, does this look right?

@phlptp
Copy link
Copy Markdown
Collaborator

phlptp commented Jan 24, 2019

yes the fix looks right.

@phlptp
Copy link
Copy Markdown
Collaborator

phlptp commented Jan 24, 2019

probably indicates there may be a missing test that would check the false branch of that if statement. If that statement was always evaluating to true (which would be by far the most common case) then it should be possible to craft a failing test case before the fix.

@henryiii
Copy link
Copy Markdown
Collaborator Author

henryiii commented Jan 24, 2019

I’m traveling till Tueday, could you make a test for it?

@phlptp phlptp mentioned this pull request Jan 24, 2019
Merged
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 24, 2019

Codecov Report

Merging #201 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #201   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        1908   1908           
=====================================
  Hits         1908   1908
Impacted Files Coverage Δ
include/CLI/StringTools.hpp 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 411364a...1777c91. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 24, 2019 •
edited
Loading

Codecov Report

Merging #201 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #201   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        1908   1908           
=====================================
  Hits         1908   1908
Impacted Files Coverage Δ
include/CLI/StringTools.hpp 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 411364a...b84a2d0. Read the comment docs.

@phlptp @henryiii
add = and : escape detect with different branch in escape_detect code…
b84a2d0 
…. The tests were verified to fail if the fix was not in place.
@henryiii henryiii merged commit f542179 into master Jan 25, 2019
@henryiii
Copy link
Copy Markdown
Collaborator Author

henryiii commented Jan 25, 2019

Thanks!

@henryiii henryiii deleted the henryiii-gcc8 branch January 30, 2019 11:37
@phlptp phlptp mentioned this pull request Feb 1, 2019
Closed
@henryiii henryiii added this to the v1.7.1 milestone Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.7.1

Development

Successfully merging this pull request may close these issues.

2 participants

@henryiii @phlptp