how to avoid macro loops

Started by
31 comments, last by rip-off 11 years, 3 months ago

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 = sLines.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 = sLines.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)

Advertisement

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

L. Spiro

I restore Nintendo 64 video-game OST’s into HD! https://www.youtube.com/channel/UCCtX_wedtZ5BoyQBXEhnVZw/playlists?view=1&sort=lad&flow=grid

ok thanks sorry for asking stupid question wacko.png

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*[]?

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<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 = 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.

This topic is closed to new replies.

Advertisement