Why am I printing more characters that i expect?

I'm trying to read a whole file as characters and then print those characters into the screen, but at the end , I'm getting some extra weird characters. Take a look at this output:

Code:

---

#version 330 core

layout(location = 0) in vec3 aPos;

void main()
{
        gl_Position = vec4(aPos, 1.0);
}═══════²²²²

---

The input file is:

Code:

---

#version 330 core

layout(location = 0) in vec3 aPos;

void main()
{
        gl_Position = vec4(aPos, 1.0);
}

---

And my source code:

Code:

---

#pragma once
#include <iostream>


class Shader
{

private:

        unsigned int m_id;

public:

        Shader(std::string vertex_filename, std::string fragment_filename);

        ~Shader();

        void Bind();

};

---

Code:

---

#include "shader.h"
#include "glcall.h"
#include "engine_error.h"
#include <fstream>
#include <string>


struct ShaderSource
{
        std::string vertex_src;
        std::string fragment_src;
};


static void ReadSources(std::string filename, bool is_vertex, struct ShaderSource *src)
{

        //Create a file object.
        std::ifstream file;

        //Open the file.
        file.open(filename, std::ios::in);

        //If the file opened successfully read it.
        if (file.is_open())
        {

                //Size of the file.
                file.seekg(0, std::ios::end);
                std::streampos size = file.tellg();
                file.seekg(0, std::ios::beg);

                //Allocate memory to store the data.
                char *data = (char *)malloc(sizeof(char) * size);

                //Read the data from the file.
                file.read(data, size);

                //Close the file.
                file.close();

                //This was the vertex file.
                if (is_vertex)
                        src->vertex_src = (const char *)data;

                //This was the fragment file.
                else
                        src->fragment_src = (const char *)data;

                //Release the memory for the data since I coppied them into the ShaderSource structure.
                free(data);
        }


        //Problem opening the file.
        else
                throw EngineError("There was a problem opening the file: " + filename);
}


Shader::Shader(std::string vertex_filename, std::string fragment_filename)
{

        //Testing if ReadSources works//
        ShaderSource source;

        ReadSources(vertex_filename, true, &source);
        std::cout << source.vertex_src;
        //Testing if ReadSources works//
}


Shader::~Shader()
{
}

void Shader::Bind()
{
        GLCall(glUseProgram(m_id));
}

---

Ah - an oldy but a goodie!

The problem here is binary versus text files. When you use .seekg() to get the size of the file, this is the actual size of the file (as shown in file listings etc). But you are opening the file as text, not binary. Hence lf/cr combinations are converted to just lf etc. So the actual size of the file read is usually smaller than the size given by .seekg(). The actual number of chars read by .read() is got by using gcount(). See http://www.cplusplus.com/reference/i...stream/gcount/ Also, you don't seem to store the size of the file data buffer anywhere - so how do you know how many bytes it contains when you come to use it??

Code:

---

//Release the memory for the data since I coppied them into the ShaderSource structure.
                free(data);

---

No you haven't. You're just set a pointer to point to the memory into which the file is read - you haven't copied the memory. So you're freeing memory that is still going to used.

PS why are using malloc()/free() in c++?? At a minimum you should be using new/delete but much better to use managed memory and use unique_ptr. See http://www.cplusplus.com/reference/memory/unique_ptr/

I added a new line to close the string a followed and everything seems to work correctly now:

Code:

---

        //If the file opened successfully read it.
        if (file.is_open())
        {

                //Size of the file.
                file.seekg(0, std::ios::end);
                std::streampos size = file.tellg();
                file.seekg(0, std::ios::beg);

                //Allocate memory to store the data.
                char *data = (char *)malloc(sizeof(char) * size );

                //Read the data from the file.
                file.read(data, size);

                //Close the string.
                data[file.gcount()] = '\0';

                //Close the file.
                file.close();

                //This was the vertex file.
                if (is_vertex)
                        src->vertex_src = (const char *)data;

                //This was the fragment file.
                else
                        src->fragment_src = (const char *)data;

                //Release the memory for the data since I coppied them into the ShaderSource structure.
                free(data);
        }

---

About free:

I thought that the equal operator was actually coping the data from the data array into the std::string so there should not be a problem freeing the data.
Also when i run the program i'm not getting any memory access violation, everything seems to work just fine.

Code:

---

//This was the vertex file.
                if (is_vertex)
                        src->vertex_src = (const char *)data;

                //This was the fragment file.
                else
                        src->fragment_src = (const char *)data;

---

Quote:

---

I thought that the equal operator was actually coping the data from the data array into the std::string

---

Sorry, I didn't see the struct at the top of the code and assumed fragment_src was a pointer. :blush: Yes, the assignment to a string does an actual copy so data can be freed.

You should still use new/delete in c++ code rather than malloc()/free().

Quote:

---

Originally Posted by 2kaud

Sorry, I didn't see the struct at the top of the code and assumed fragment_src was a pointer. :blush: Yes, the assignment to a string does an actual copy so data can be freed.

You should still use new/delete in c++ code rather than malloc()/free().

---

Yeah you are probably right, but i guess if i'm not allocating an object malloc should work just fine because i don't need to call o constructor.

One mistake i made one time was this:

Code:

---

typedef struct ShaderSource
{
    std::string s1;
    std::string s2;
}Source;

Source *src = (ShaderSource *)malloc(sizeof(ShaderSource));

src->s1 = "someting";
src->s2 = "someting";

---

I was getting memory access violation probably because the constructor of the std::string was not get called because i was using malloc instead of new.

Quote:

---

i guess if i'm not allocating an object malloc should work just fine because i don't need to call o constructor.

---

Yes - in this case. But why not get into the habit of always using new/delete - or even better unique_ptr/shared_ptr.

Quote:

---

Originally Posted by 2kaud

Yes - in this case. But why not get into the habit of always using new/delete - or even better unique_ptr/shared_ptr.

---

Well, if i told you that i just understood the meaning of smart pointers would you believe me :D I couldn't see why you need a pointer that will get free automatically when you reach out of scope but with this code now i got it.

Great! Another convert :)

Actually in your code you don't need to directly allocate any memory at all! You can let string take all the strain. Consider

Code:

---

//Size of the file.
file.seekg(0, std::ios::end);
std::streampos size = file.tellg();
file.seekg(0, std::ios::beg);

std::string data(size, 0);


//Read the data from the file.
file.read(data.data(), size);

//Close the string.
data[file.gcount()] = '\0';
data.resize(file.gcount());
//Close the file.
file.close();

---

Quote:

---

Originally Posted by 2kaud

Great! Another convert :)

Actually in your code you don't need to directly allocate any memory at all! You can let string take all the strain. Consider

Code:

---

//Size of the file.
file.seekg(0, std::ios::end);
std::streampos size = file.tellg();
file.seekg(0, std::ios::beg);

std::string data(size, 0);


//Read the data from the file.
file.read(data.data(), size);

//Close the string.
data[file.gcount()] = '\0';
data.resize(file.gcount());
//Close the file.
file.close();

---

---

That's a little tricky :D I read somewhere:

Accessing the value at data()+size() produces undefined behavior: There are no guarantees that a null character terminates the character sequence pointed by the value returned by this function. See string::c_str for a function that provides such guarantee.

A program shall not alter any of the characters in this sequence.

but i guess if you know what you are doing is fine.

With c++17, the pointer returned by .data() is now non-const so the memory referenced by .data() can be altered. The string constructor creates a string of at least the required size so the .read() will happily read the contents of the file into the string memory. Note this is c++17. Previous c++ standards have .data() returning a const pointer so the string memory can't (shouldn't) be altered like this.

Quote:

---

Originally Posted by 2kaud

With c++17, the pointer returned by .data() is now non-const so the memory referenced by .data() can be altered. The string constructor creates a string of at least the required size so the .read() will happily read the contents of the file into the string memory. Note this is c++17. Previous c++ standards have .data() returning a const pointer so the string memory can't (shouldn't) be altered like this.

---

Good to know :)

There's been a change in C++17. .c_str() returns a const pointer to a null terminated string (as previously). .data() now returns a non-const pointer to the internal string memory which is not guaranteed to be null-terminated (although in practice it is but according to the standard can't be assumed). In previous c++ standards, .data() was the same as .c_str() - now it's not.

.size() gives the number of chars currently stored in the internal memory and .capacity() gives the maximum number that can be held with the currently allocated memory. .size() must be <= .capacity(). If using .data() to amend string data, don't alter more than .capacity() chars otherwise you will overflow the buffer and cause problems.

Quote:

---

value at data()+size() produces undefined behavior:

---

Yes, the code in post #8 probably should be

Code:

---

//Size of the file.
file.seekg(0, std::ios::end);
std::streampos size = file.tellg();
file.seekg(0, std::ios::beg);

std::string data(size + 1, 0);


//Read the data from the file.
file.read(data.data(), size);

//Close the string.
data[file.gcount()] = '\0';
data.resize(file.gcount() + 1);
//Close the file.
file.close();

---