Jump to content

  • Log In with Google      Sign In   
  • Create Account

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.

  • You cannot reply to this topic
33 replies to this topic

#1 proanim   Members   -  Reputation: 440

Like
0Likes
Like

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)

 



Sponsor:

#2 L. Spiro   Crossbones+   -  Reputation: 13599

Like
2Likes
Like

Posted 25 December 2012 - 08:10 PM

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

 

 

L. Spiro


It is amazing how often people try to be unique, and yet they are always trying to make others be like them. - L. Spiro 2011
I spent most of my life learning the courage it takes to go out and get what I want. Now that I have it, I am not sure exactly what it is that I want. - L. Spiro 2013
I went to my local Subway once to find some guy yelling at the staff. When someone finally came to take my order and asked, “May I help you?”, I replied, “Yeah, I’ll have one asshole to go.”
L. Spiro Engine: http://lspiroengine.com
L. Spiro Engine Forums: http://lspiroengine.com/forums

#3 proanim   Members   -  Reputation: 440

Like
0Likes
Like

Posted 25 December 2012 - 08:34 PM

ok thanks sorry for asking stupid question wacko.png  



#4 Servant of the Lord   Crossbones+   -  Reputation: 19556

Like
5Likes
Like

Posted 26 December 2012 - 08:36 AM

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' 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 - [indie turn-based rpg set in a para-historical French colony] | Indie RPG development journal

[Fly with me on Twitter] [Google+] [My broken website]

[Need web hosting? I personally like A Small Orange]


#5 samoth   Crossbones+   -  Reputation: 4783

Like
3Likes
Like

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.



#6 JTippetts   Moderators   -  Reputation: 8492

Like
2Likes
Like

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.



#7 proanim   Members   -  Reputation: 440

Like
0Likes
Like

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.


#8 JTippetts   Moderators   -  Reputation: 8492

Like
5Likes
Like

Posted 26 December 2012 - 02:44 PM

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.



#9 proanim   Members   -  Reputation: 440

Like
0Likes
Like

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

	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.



#10 JTippetts   Moderators   -  Reputation: 8492

Like
0Likes
Like

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.



#11 proanim   Members   -  Reputation: 440

Like
0Likes
Like

Posted 26 December 2012 - 05:35 PM

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



#12 L. Spiro   Crossbones+   -  Reputation: 13599

Like
2Likes
Like

Posted 26 December 2012 - 05:41 PM

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


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

It is amazing how often people try to be unique, and yet they are always trying to make others be like them. - L. Spiro 2011
I spent most of my life learning the courage it takes to go out and get what I want. Now that I have it, I am not sure exactly what it is that I want. - L. Spiro 2013
I went to my local Subway once to find some guy yelling at the staff. When someone finally came to take my order and asked, “May I help you?”, I replied, “Yeah, I’ll have one asshole to go.”
L. Spiro Engine: http://lspiroengine.com
L. Spiro Engine Forums: http://lspiroengine.com/forums

#13 santa01   Members   -  Reputation: 307

Like
5Likes
Like

Posted 26 December 2012 - 06:10 PM

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

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


#14 proanim   Members   -  Reputation: 440

Like
0Likes
Like

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

 

 

 

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


#15 L. Spiro   Crossbones+   -  Reputation: 13599

Like
0Likes
Like

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
It is amazing how often people try to be unique, and yet they are always trying to make others be like them. - L. Spiro 2011
I spent most of my life learning the courage it takes to go out and get what I want. Now that I have it, I am not sure exactly what it is that I want. - L. Spiro 2013
I went to my local Subway once to find some guy yelling at the staff. When someone finally came to take my order and asked, “May I help you?”, I replied, “Yeah, I’ll have one asshole to go.”
L. Spiro Engine: http://lspiroengine.com
L. Spiro Engine Forums: http://lspiroengine.com/forums

#16 Cornstalks   Crossbones+   -  Reputation: 6990

Like
0Likes
Like

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 ]

#17 proanim   Members   -  Reputation: 440

Like
0Likes
Like

Posted 27 December 2012 - 09:17 AM

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


#18 Brother Bob   Moderators   -  Reputation: 8196

Like
1Likes
Like

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.


#19 proanim   Members   -  Reputation: 440

Like
0Likes
Like

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?



#20 L. Spiro   Crossbones+   -  Reputation: 13599

Like
2Likes
Like

Posted 27 December 2012 - 10:23 AM

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

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

It is amazing how often people try to be unique, and yet they are always trying to make others be like them. - L. Spiro 2011
I spent most of my life learning the courage it takes to go out and get what I want. Now that I have it, I am not sure exactly what it is that I want. - L. Spiro 2013
I went to my local Subway once to find some guy yelling at the staff. When someone finally came to take my order and asked, “May I help you?”, I replied, “Yeah, I’ll have one asshole to go.”
L. Spiro Engine: http://lspiroengine.com
L. Spiro Engine Forums: http://lspiroengine.com/forums




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.



PARTNERS