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

Fixed image file extension detection #931

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@MarioLiebisch
Member

MarioLiebisch commented Jul 23, 2015

  • Previously this would fail on jpeg and would detect something such as notajpg as jpg.
  • This fixes issue #929.
if (filename.size() > 3)
// Extract the extension
const std::size_t dot = filename.find_last_of('.');

This comment has been minimized.

@kimci86

kimci86 Jul 23, 2015

Contributor

To nitpick, I think that using rfind could be more appropriate.

@kimci86

kimci86 Jul 23, 2015

Contributor

To nitpick, I think that using rfind could be more appropriate.

This comment has been minimized.

@LaurentGomila

LaurentGomila Jul 23, 2015

Member

To nitpick, I think that using rfind could be more appropriate.

Why? For a single character it should make absolutely no difference.

@LaurentGomila

LaurentGomila Jul 23, 2015

Member

To nitpick, I think that using rfind could be more appropriate.

Why? For a single character it should make absolutely no difference.

This comment has been minimized.

@kimci86

kimci86 Jul 23, 2015

Contributor

In my opinion, it looks weird to use find_last_of with a single character. I think rfind make more sense. Anyway feel free to ignore this comment.

@kimci86

kimci86 Jul 23, 2015

Contributor

In my opinion, it looks weird to use find_last_of with a single character. I think rfind make more sense. Anyway feel free to ignore this comment.

This comment has been minimized.

@MarioLiebisch

MarioLiebisch Jul 23, 2015

Member

Never really used rfind() (didn't actually know it :D). I guess for single characters the implementation is exactly the same. The only difference occurs when passing an actual string.

@MarioLiebisch

MarioLiebisch Jul 23, 2015

Member

Never really used rfind() (didn't actually know it :D). I guess for single characters the implementation is exactly the same. The only difference occurs when passing an actual string.

// Extract the extension
const std::size_t dot = filename.find_last_of('.');
const std::string extension = dot != std::string::npos ? toLower(filename.substr(dot + 1)) : "";

This comment has been minimized.

@kimci86

kimci86 Jul 23, 2015

Contributor

I think the check for a valid dot index should be done first in an if embracing all the following code. What do you think ?

if (dot != std::string::npos)
{
    const std::string extension = toLower(filename.substr(dot + 1));
    if (extension == "bmp")
    //...
}
@kimci86

kimci86 Jul 23, 2015

Contributor

I think the check for a valid dot index should be done first in an if embracing all the following code. What do you think ?

if (dot != std::string::npos)
{
    const std::string extension = toLower(filename.substr(dot + 1));
    if (extension == "bmp")
    //...
}

This comment has been minimized.

@LaurentGomila

LaurentGomila Jul 23, 2015

Member

Let's not make optimization or special cases where we don't need to. "" is a valid extension, like any other.

@LaurentGomila

LaurentGomila Jul 23, 2015

Member

Let's not make optimization or special cases where we don't need to. "" is a valid extension, like any other.

This comment has been minimized.

@MarioLiebisch

MarioLiebisch Jul 23, 2015

Member

I thought about something similar as well, but I don't think it's worth making the code more elaborate. Also your snippet is excluding the case where there actually is a dot, but no extension (the file name could be image.). So you could actually check for an empty extension first to skip all further checks, but at the same time that's marginal complexity compared to everything else happening in that whole call.

@MarioLiebisch

MarioLiebisch Jul 23, 2015

Member

I thought about something similar as well, but I don't think it's worth making the code more elaborate. Also your snippet is excluding the case where there actually is a dot, but no extension (the file name could be image.). So you could actually check for an empty extension first to skip all further checks, but at the same time that's marginal complexity compared to everything else happening in that whole call.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jul 24, 2015

Member

Looks good. 👍 🐈

Member

TankOs commented Jul 24, 2015

Looks good. 👍 🐈

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 29, 2015

Member

👍

Member

binary1248 commented Jul 29, 2015

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 4, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon into the branch 2.3.x, unless someone raises any concerns.

Member

eXpl0it3r commented Aug 4, 2015

This PR has been added to my merge list, meaning it will be merged soon into the branch 2.3.x, unless someone raises any concerns.

Kipernal and others added some commits Jul 14, 2015

Fixed image file extension detection
Previously this would fail on `jpeg` and would detect something such as
`notajpg` as `jpg`. This fixes #929.
@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 12, 2015

Member

Merged in df99d5f on branch 2.3.x

Member

eXpl0it3r commented Aug 12, 2015

Merged in df99d5f on branch 2.3.x

@eXpl0it3r eXpl0it3r closed this Aug 12, 2015

@eXpl0it3r eXpl0it3r deleted the bugfix/image-writer-extensions branch Aug 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment