View more

View more

View more

### Image of the Day Submit

IOTD | Top Screenshots

### The latest, straight to your Inbox.

Subscribe to GameDev.net Direct to receive the latest updates and exclusive content.

# how to avoid macro loops

Old topic!

Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

33 replies to this topic

### #1proanim  Members

Posted 25 December 2012 - 07:53 PM

If found some source code that has this in it

#define FOR(q,n) for(int q=0;q<n;q++)
#define SFOR(q,s,e) for(int q=s;q<=e;q++)
#define RFOR(q,n) for(int q=n;q>=0;q--)
#define RSFOR(q,s,e) for(int q=s;q>=e;q--)
#define ESZ(elem) (int)elem.size()

and it is used as

const char** sProgram = new const char*[ESZ(sLines)];
FOR(i, ESZ(sLines))sProgram[i] = sLines[i].c_str();

How should i make these loops so i dont have to use these macros?
I tried this

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

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

but the for loop gets vector out of bounds error when it hits last line in file (this is used in function that reads text file)

### #2L. Spiro  Members

Posted 25 December 2012 - 08:10 PM

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

L. Spiro

### #3proanim  Members

Posted 25 December 2012 - 08:34 PM

ok thanks sorry for asking stupid question

### #4Servant of the Lord  Members

Posted 26 December 2012 - 08:36 AM

POPULAR

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<std::string> to char*[]?

It's perfectly fine to abbreviate my username to 'Servant' or 'SotL' rather than copy+pasting it all the time.
All glory be to the Man at the right hand... On David's throne the King will reign, and the Government will rest upon His shoulders. All the earth will see the salvation of God.
Of Stranger Flames -

### #5samoth  Members

Posted 26 December 2012 - 11:50 AM

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.

### #6JTippetts  Moderators

Posted 26 December 2012 - 01:06 PM

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.

### #7proanim  Members

Posted 26 December 2012 - 02:12 PM

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

// get all lines from a file
vector<string> 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[i] = sLines[i].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?

Edited by proanim, 26 December 2012 - 02:12 PM.

### #8JTippetts  Moderators

Posted 26 December 2012 - 02:44 PM

POPULAR

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[i].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.

### #9proanim  Members

Posted 26 December 2012 - 04:47 PM

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());

//delete[] sProgram;

int iCompilationStatus;

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

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.

### #10JTippetts  Moderators

Posted 26 December 2012 - 04:54 PM

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

### #11proanim  Members

Posted 26 December 2012 - 05:35 PM

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

### #12L. Spiro  Members

Posted 26 December 2012 - 05:41 PM

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

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

Edited by L. Spiro, 26 December 2012 - 05:45 PM.

### #13santa01  Members

Posted 26 December 2012 - 06:10 PM

POPULAR

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);

GLint compileStatus;
if (compileStatus == GL_FALSE) {
GLint infoLogLength;

GLchar* infoLog = new GLchar[infoLogLength + 1];

Logger::getInstance().log(Logger::LOG_ERROR, "In file %s: %s", name.c_str(), infoLog);
delete[] infoLog;
}

}

Edited by santa01, 26 December 2012 - 06:12 PM.

### #14proanim  Members

Posted 26 December 2012 - 07:23 PM

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

#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;
}

#version 330

smooth in vec3 theColor;
out vec4 outputColor;

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

// Load shaders and create shader program


### #15L. Spiro  Members

Posted 26 December 2012 - 08:10 PM

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

### #16Cornstalks  Members

Posted 26 December 2012 - 09:49 PM

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

Edited by Cornstalks, 26 December 2012 - 10:00 PM.

[ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

### #17proanim  Members

Posted 27 December 2012 - 09:17 AM

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

int infologLength;
int maxLength;
char* infoLog;

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

### #18Brother Bob  Moderators

Posted 27 December 2012 - 09:27 AM

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.

### #19proanim  Members

Posted 27 December 2012 - 09:44 AM

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?

### #20L. Spiro  Members

Posted 27 December 2012 - 10:23 AM

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 […]”.

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

Edited by L. Spiro, 27 December 2012 - 10:36 AM.

Old topic!

Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.