Contradiction In OpenGLShader::PreProcess

[MrBrN197]

In PreProcess function, inside the while loop on the last substr. The second parameter checks to see if nextLinePos == std::string::npos and returns source.size() - 1. I'm not sure how this makes sense, furthermore we are passing nextLinePos as the first parameter; It seems if you pass std::string::npos as the first parameter of substr the program crashes, the second parameter checking for nextLinePos == std::string::npos doesn't make sense. but I've not hard anyone complaining about this so maybe I don't understand what is really going on here?

std::unordered_map<GLenum, std::string> OpenGLShader::PreProcess(const std::string& source)
{
    std::unordered_map<GLenum, std::string> shaderSources;

    const char* typeToken = "#type";
    size_t typeTokenLength = strlen(typeToken);
    size_t pos = source.find(typeToken, 0);
    while (pos != std::string::npos)
    {
        size_t eol = source.find_first_of("\r\n", pos);
        HZ_CORE_ASSERT(eol != std::string::npos, "Syntax error");
        size_t begin = pos + typeTokenLength + 1;
        std::string type = source.substr(begin, eol - begin);
        HZ_CORE_ASSERT(ShaderTypeFromString(type), "Invalid shader type specified");

        size_t nextLinePos = source.find_first_not_of("\r\n", eol);
        pos = source.find(typeToken, nextLinePos);
        shaderSources[ShaderTypeFromString(type)] = source.substr(nextLinePos, pos - (nextLinePos == std::string::npos ? source.size() - 1 : nextLinePos));
    }

    return shaderSources;
}

If you create a shader that specifies a tokenType like #vertex and the file ends on that line I think the program would crash. I would ASSERT this and throw a syntax error message for this as well.

[CleverSource]

You should make a PR which implements this.

[LovelySanta]

In PreProcess function, inside the while loop on the last substr. The second parameter checks to see if nextLinePos == std::string::npos and returns source.size() - 1. I'm not sure how this makes sense, furthermore we are passing nextLinePos as the first parameter;

Inside the while loop:

1. pos contains the start of the shader, at the start of the #type shader line
2. we look for eol, the end of the line, at the end of the #type shader line, if that is npos, we should've gone outside of the loop as we've reached the end... (hence the assert)
3. we extract the type (and assert it) that we found using the line inbetween pos and eol that we found in step 1 and 2.
4. nextLinePos contains the position at the start of the first line of the actual shader, and is the start to search. Note: this will NEVER be npos, as that would mean we are at the end of the string, which was asserted in step 2.
5. we search pos to be the end of the shader code (aka the start of the next #type shader line or npos if the shader code goes to the end of the string).
6. If step 5 found npos, we change this to the end of the string, otherwise we just return the position we found.

It seems if you pass std::string::npos as the first parameter of substr the program crashes, the second parameter checking for nextLinePos == std::string::npos doesn't make sense. but I've not hard anyone complaining about this so maybe I don't understand what is really going on here?

See my explanation above (number 4), it can never be npos as it would've been asserted before.

If you create a shader that specifies a tokenType like #vertex and the file ends on that line I think the program would crash. I would ASSERT this and throw a syntax error message for this as well.

If the file would end with the #type shader at the end, the eol would have the value npos (regardless if it had the \r\n at that last line).
If there was some character on the next line, the shader would be at least the size of 1 character long, which would just create a compile error on the shader and not an error in the PreProcess function.

Am I missing something here (as in, is there an actual bug), or did I clearify the inner workings a bit more in depth? Since I won't take a conclusion now, I won't mark this as a bug, nor closing it as not a bug (for now).

Kind regards
lovely_santa

[MrBrN197]

@LovelySanta Ok, so thank you for trying to explain this to me.

See my explanation above (number 4), it can never be npos as it would've been asserted before

The initial reason for me creating this issue was rooted in the nextLinePos variable. Your reply Is explaining how the PreProcess(which to be fair I also wanted clarification on) but the glaring issue for me is this specific ternary statement nextLinePos==std::string::npos in the parameter of substr.

I said It seems you can't pass std::string::npos to the substr function (the program crashes) and we should assert nextLinePos to ensure it is not std::string::npos but you've told me I shouldn't do that because nextLinePos can never be std::string::npos; so if that is the case why is the function still checking if nextLinePos is equal to std::string::npos. Do you see what I mean. To me that is, if not a bug, a contradiction at least.Furthermore the idea that doing pos - std::string::npos gives something sensible, I just don't believe.

Secondly. I do believe that nextLinePos can in fact be std::string::npos without an ASSERT being fired. we don't ever assert that it is not std::string::npos so if we don't find a next position in the string that is not "\r\n", nextLinePos will be std::string::npos. You see what I'm saying.

Just for reference this is how it would be written for me.

size_t nextLinePos = source.find_first_not_of("\r\n", eol);
HZ_ASSERT(nextLinePos != std::string::npos, "Syntax Error");

pos = source.find(typeToken, nextLinePos);
shaderSources[ShaderTypeFromString(type)] = source.substr(nextLinePos, (pos == std::string::npos ? source.size() - nextLinePos : pos - nextLinePos));

[LovelySanta]

Two things:

1. You're right, I didn't notice it, but we have to check if pos equals npos instead of nextLinePos!

pos = source.find(typeToken, nextLinePos);
shaderSources[ShaderTypeFromString(type)] = source.substr(nextLinePos, (pos == std::string::npos ? source.size() - 1 : pos) - nextLinePos);

2. Yes, it might not be a bad idea to assert the nextLinePos to be different of npos.

The first is an issue for sure, the 2nd is more a suggestion. I'll mark this as a bug, and feel free to make a PR to implement both.