how to avoid macro loops

for(i; i < sLines.size(); i++)

L. Spiro

ok thanks sorry for asking stupid question

Also, you can put the int inside the for() statement:

for(int i; i < sLines.size(); i++)

The difference is whether 'i' goes in the scope outside the for() loop or not. Usually you don't want that, but occasionally you do.

The code seems to be converting an std::vector of std::strings to a dynamically allocated raw array of raw strings. That's like going from driving a good car with airbags and your seatbelt on (perfectly safe), to ducktaping yourself to the belly of an elephant (dangerous, messy, and in most situations just plain dumb). Are you sure the code you are converting actually needs to make the std::vector to char*[]?

In case you use GCC, you can compile with -save-temps to trivially figure out any macro abuse, no matter how convoluted it is. For every file foo.c you will find a file foo.ii generated, which contains the output of the preprocessor (i.e. with everything included, and all macros expanded). I've found this immensely helpful at times.

I'm with Servant of the Lord on this. Are you really sure you need to do this?

Sometimes you have to question the quality of your sources. In this case, it's not only converting away from std::string to unsafe raw strings that is a code smell, but the use of macros in this situation also smells nasty. Macro usage like this can conceal or even introduce bugs, and detracts from readability, making it a confusing mess for no real reason other than somebody thought they could save some typing. If they took shortcuts there, the remainder of their code might be smelly, too.

What is it you are trying to accomplish in the larger scheme? Maybe we can help you figure out a less smelly method.

Well it is 'simple' process - the function opens file for reading, reads through entire file with this

// get all lines from a file
vector sLines;
char sLine[255];
while(fgets(sLine, 255, fp))sLines.push_back(sLine);
fclose(fp);

then it does this

const char** sProgram = new const char* [(int)sLines.size()];

int i = 0;
for(i; i < sLines.size(); i++)
sProgram = sLines.c_str();

and processes entire file as needed and does

delete[] sProgram;

now when i look at this I see that I never free sLines vector is that needed?

You're mixing things up here.

First of all, fgets() is a C function (included via the header cstdio). You are reading from a file into a C-style array, then creating a C++ string from the line you read. This is pointless. C++ includes functionality for reading lines from files as well, without the limitation of reading to a fixed-length buffer:

#include <fstream> #include <string> std::vector<std::string> sLines; std::ifstream infile("testfile.txt"); while(!infile.eof()) { std::string s; std::getline(infile, s); sLines.push_back(s); } infile.close();

getline() will read a string until a newline is encountered, without the maximum size limit of 255 that the fixed-length C-style read has. The allocation is invisibly handled by the string class.

And no, you don't need to free sLines explicitly; it will be freed when it goes out of scope. The only things you need to free are things you explicitly new. That's the beauty of using C++ standard library functionality, rather than C or (as you are doing here) a bastard mix of C and C++.

Now, I take it from the second part of your code that you need sProgram to be an array of pointers to C-style strings (for whatever reason). Rather than explicitly using new/delete you could do something like this (note that this is still somewhat C-ish, and there are other and more concise ways of doing it in straight C++; but now is not the time to introduce lambdas, I think ;) ):

std::vector<const char *> sProgram; for(unsigned int i=0; i<sLines.size(); ++i) { sProgram.push_back(sLines.c_str()); }

If you have C++11 you can then call sProgram.data() to get a pointer to the array of const char * held by sProgram, and use that wherever you are using your current sProgram pointer. If you don't have C++11, then the line &sProgram[0] works, though it is, of course, a bit hacky, (and the pointer is invalidated if you do another push_back that causes a reallocation, so be careful). This way, sProgram is also automatically managed (it grows as needed when more lines are added, and is automatically deleted when it goes out of scope) without the explicit new/delete calls. Even better would be to re-work whatever it is you need sProgram for so it doesn't require an array of C-style strings, but this might not always be possible if you are sending it to a third party library or API (such as a shader compiler, which I suspect you are doing).

In this day and age, if you are using a C++ compiler and you are doing explicit allocating/de-allocating of arrays and strings, then you really ought to take a good hard look at what you are doing and why, as there are almost always safer ways to do it that don't require possibly unsafe memory management.

Ok i made changes and now same stuff happens only difference is now i can't complete the entire process. There is no error or anything the funtion returns false

bool CShader::loadShader(string sFile, int a_iType)
{
	//FILE* fp = fopen(sFile.c_str(), "rt");
	//if(!fp)return false;

	// get all lines from a file
	vector<string> sLines;
	//char sLine[255];
	//while(fgets(sLine, 255, fp))sLines.push_back(sLine);
	//fclose(fp);

	ifstream infile(sFile.c_str());

	while(!infile.eof())
	{
		string s;
		getline(infile, s);
		sLines.push_back(s);
	}

	infile.close();

	//const char** sProgram = new const char*[ESZ(sLines)];
	//FOR(i, ESZ(sLines))sProgram[i] = sLines[i].c_str();
	
	//const char** sProgram = new const char* [(int)sLines.size()];
	vector<const char *> sProgram;

	//for(int i; i < sLines.size(); i++)
	//	sProgram[i] = sLines[i].c_str();
	for(unsigned int i=0; i<sLines.size(); ++i)
		sProgram.push_back(sLines[i].c_str());

	uiShader = glCreateShader(a_iType);

	glShaderSource(uiShader, sLines.size(), &sProgram[0], NULL);
	glCompileShader(uiShader);

	//delete[] sProgram;

	int iCompilationStatus;
	glGetShaderiv(uiShader, GL_COMPILE_STATUS, &iCompilationStatus);

	if(iCompilationStatus == GL_FALSE)return false;
	iType = a_iType;
	bLoaded = true;

	return 1;
}

What can cause glGetShaderiv(uiShader, GL_COMPILE_STATUS, &iCompilationStatus); to fail ? Shader files that i use with this are basic color shaders and they work with orginal function but now they fail to compile, I am not sure why exactly.

Per the manual page on glCompileShader you can call glGetShaderInfoLog to get some information on why the compile might have failed.

Well all I can get from glGetShaderInfoLog is '00B4A520' or similar ???

Why don’t you show the code you are using to get said results?

Also, your way of loading the shader is dreadful (why not just load the file into memory, add a 0 at the end, and pass that directly to ::glShaderSource() instead of wasting time creating an array of char *’s?), but I don’t see any bugs in it.

So why not save a reply and also post the shader code since that seems more likely to be the source of the error?

L. Spiro

Why do you guys load shader sources line by line? Thats a extra runtime. Shader sources aren't very big, everything can be done with one read:

GLuint ShaderLoader::shaderFromFile(const std::string& name, GLenum type) {
    std::fstream file(name.c_str(), std::ios::binary | std::ios::in);
    if (!file.good()) {
        Logger::getInstance().log(Logger::LOG_ERROR, "Cannot open file %s", name.c_str());
        return 0;
    }

    file.seekg(0, std::ios::end);
    int sourceLength = file.tellg();
    GLchar* shaderSource = new GLchar[sourceLength + 1];

    file.seekg(0, std::ios::beg);
    file.read(shaderSource, sourceLength);
    shaderSource[sourceLength] = 0;

    GLuint shader = glCreateShader(type);
    glShaderSource(shader, 1, (const GLchar**)&shaderSource, NULL);
    glCompileShader(shader);

    delete[] shaderSource;

    GLint compileStatus;
    glGetShaderiv(shader, GL_COMPILE_STATUS, &compileStatus);
    if (compileStatus == GL_FALSE) {
        GLint infoLogLength;
        glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &infoLogLength);
        
        GLchar* infoLog = new GLchar[infoLogLength + 1];
        glGetShaderInfoLog(shader, infoLogLength, NULL, infoLog);
        
        Logger::getInstance().log(Logger::LOG_ERROR, "In file %s: %s", name.c_str(), infoLog);
        delete[] infoLog;
    }
    
    return shader;
}

So why not save a reply and also post the shader code since that seems more likely to be the source of the error?

Shaders are working in first version of the function

vertex shader:

#version 330

layout (location = 0) in vec3 inPosition;
layout (location = 1) in vec3 inColor;

smooth out vec3 theColor;

void main()
{
	gl_Position = vec4(inPosition, 1.0);
	theColor = inColor;
}

fragment shader

#version 330

smooth in vec3 theColor;
out vec4 outputColor;

void main()
{
	outputColor = vec4(theColor, 1.0);
}

these are loaded with

// Load shaders and create shader program
shVertex.loadShader("data/shaders/color.vert", GL_VERTEX_SHADER);
shFragment.loadShader("data/shaders/color.frag", GL_FRAGMENT_SHADER);

Well all I can get from glGetShaderInfoLog is '00B4A520' or similar ???

Why don’t you show the code you are using to get said results?

L. Spiro

What L. Spiro is saying, in case it's not obvious, is that you're not showing how you're getting this error log back... Your earlier post shows some good info on how you're compiling your shader, but it doesn't show how you're obtaining that strange error log.

It's kinda like trying to diagnose a patient, if you're a doctor, and he says "I have symptoms" but doesn't say what those symptoms are :)

glShaderSource(uiShader, sLines.size(), &sProgram[0], NULL);
glCompileShader(uiShader);

int infologLength;
int maxLength;
char* infoLog;

glGetShaderiv(uiShader,GL_INFO_LOG_LENGTH, &maxLength);
glGetShaderInfoLog(uiShader, maxLength, &infologLength, infoLog);

ofstream outfile;
outfile.open ("log.txt");
outfile << &infoLog << endl;
outfile.close();

Santa01 showed above how to get the log from the shader or program object. Your code has two major problems though:

1. glGetShaderInfoLog expect you to allocate the buffer so that it can write the log to it. You're not allocating anything and so the function is writing to wherever the pointer happens to point. In this case, it's trashing the stack big time.
2. infoLog is char *, and so infoLog itself is the string containing the log, not &intoLog. Output as a string is only overloaded for char * (with combinations of cv-qualifiers), but &infoLog is a char ** which just prints the address of the infoLog variable.

finally, so the errors are this

Fragment shader failed to compile with the following errors:
ERROR: 2:1: error(#307) Profile "smooth" is not supported
ERROR: 2:1: error(#76) Syntax error: unexpected tokens following #version
ERROR: error(#273) 2 compilation errors.  No code generated

any ideas?

My guess is the dreadful mess of code you are using to load the shader text.

Specifically:

getline(infile, s);

should be:

getline(infile, s);
s.append( endl );

I had assumed originally that ::glShaderSource() would treat each line you passed to it as…a line.
But closer reading reveals that all it does is put all the lines back together just as you pass them to the function, which means if you don’t have line endings then it will be passed to the compiler as a shader written entirely on one line. The reason it sees “330” and “smooth” as separate identifiers is because there is a hidden \r between them that was not discarded by getline(), but inside the shader/compiler that does not constitute a new line, so it basically sees this: “#version 330 smooth in vec3 […]”.

This is exactly why I call your loading code a dreadful mess.
You load the whole file, expand it into an array of lines (which removes the \n line-ending), make another array of pointers into that, then pass that whole thing to OpenGL, and all OpenGL does internally is make it all back into one single long line of text, which is exactly what you had (minus new lines) when you loaded it directly from the file in the first place before you did all that mess of array creation and line-by-line parsing.

You basically spent a lot of time writing a complex loader whose only purpose is to remove new-lines and create bugs.
Why not do as I suggested and just load the file directly to memory, append a 0, and pass it as-is with no further parsing to OpenGL?
santa01 showed you how to do it. It is 10 times less complex, 100 times faster, and 10 times less error-prone.


L. Spiro