|
|
I need opinions on code like this:
"if ((++(m_pLF->m_iLogCnt)) >= MSG_QUEUE_FILE_SIZE)"
maybe it's just me and my past - I was taught never to depend on order of operations. This is handled with the parenthesis , but really, should it be that hard to analyze an IF statement?
Charlie Gilley
<italic>Stuck in a dysfunctional matrix from which I must escape...
"Where liberty dwells, there is my country." B. Franklin, 1783
“They who can give up essential liberty to obtain a little temporary safety deserve neither liberty nor safety.” BF, 1759
|
|
|
|
|
That's not terribly complex, but did you really need it that way?
|
|
|
|
|
No, that one was not terribly complex, and I also selectively edited it.
Charlie Gilley
<italic>Stuck in a dysfunctional matrix from which I must escape...
"Where liberty dwells, there is my country." B. Franklin, 1783
“They who can give up essential liberty to obtain a little temporary safety deserve neither liberty nor safety.” BF, 1759
|
|
|
|
|
Obfuscation is the best way to annoy those who have to maintain your code.
|
|
|
|
|
You know, it's one thing to obfuscate, it's another to code in such a style as approaching enemy action. It's hard enough to write clear code. The style of code I posted just seems easy to break and difficult to debug. The debugger is going to treat that like one line of code.
Caveat: I freely admit that I choose to restrict how much code I put on a line. I like using ternary or conditional operator as it can make things much cleaner, as long as you don't get silly with it.
Charlie Gilley
<italic>Stuck in a dysfunctional matrix from which I must escape...
"Where liberty dwells, there is my country." B. Franklin, 1783
“They who can give up essential liberty to obtain a little temporary safety deserve neither liberty nor safety.” BF, 1759
|
|
|
|
|
Absolutely agree with you. Most of the time code like that is done by people who don't know any better (copied it from the internet), or who think they are so damn clever.
|
|
|
|
|
Well, I know who wrote the code - I'll go with clever. Bright individual, I just wanted to make sure it wasn't just me.
Charlie Gilley
<italic>Stuck in a dysfunctional matrix from which I must escape...
"Where liberty dwells, there is my country." B. Franklin, 1783
“They who can give up essential liberty to obtain a little temporary safety deserve neither liberty nor safety.” BF, 1759
|
|
|
|
|
This is not too complex, but at the limit where I would consider breaking it up into multiple statements.
That said, if that were a while condition, I might be more inclined to leave it like that, because (a) the increment might be considered part of the loop iteration, and (b) moving part of the condition might require more than one additional line (e. g. once before the start of the loop and once inside).
Apart from that, I am more bothered with the naming
GOTOs are a bit like wire coat hangers: they tend to breed in the darkness, such that where there once were few, eventually there are many, and the program's architecture collapses beneath them. (Fran Poretto)
|
|
|
|
|
I am going crazy over this simple test code.
char *Result ={"Result init "};
char *Source = {"123"};
char *Destination = {"456"};
I can verify that all strings are as initialized.
Now I run this strcpy
Result = strcpy(Source,Destination);
Now I can verify, using same approach as before , that
Result = 123 - AKA orignal Source, it was Result init before strcpy was run.
and Source is STILL 123 .
Result suppose to be Source ( source destination swap) , but I expect the Source to be COPY of the original Destination AKA 456
What am I doing wrong?
strcpy executes giving correct return, but unmodified Destination.
Am I doing the initialization correctly?
Am I nuts?
Standard stdio using GCC compiler /tools on Arduino IDE.
Thank for your help.
Vaclav
modified 3-Aug-15 22:30pm.
|
|
|
|
|
I think you have the parameter names backward so it's confusing to me.
After the strcpy, Result and Source should contain the same address, the value there should be 465, and the value at Destination should also be 456.
Is that what you see? Is that not what you want?
Be aware that you are not implementing a "swap" at all.
Unless there are other concerns, if you want to "swap" then swap the pointers, not the values:
Result = Destination;
Destination = Source;
Source = Result;
modified 3-Aug-15 23:29pm.
|
|
|
|
|
It should not work at all. String constants declared as above are read-only, so you should not be able to use strcpy to overwrite one of them by another. When I try it I get an access violation on the write, as I would expect. I am not sure why your system does not give the fault, maybe a peculiarity of Arduino. But since strcpy is supposed to return the pointer to the destination field, Result will now point to the data that Source points to, i.e. "123", but the original data should be untouched.
|
|
|
|
|
You cannot do that, it is a mistake. Your variables point to statically allocated (i.e. constant) strings you must NOT try to change.
The following program
#include <stdio.h>
#include <string.h>
int main()
{
char Result[] ="Result init ";
char Source[] = "123";
char Destination[] = "456";
strcpy(Destination, Source);
printf("%s %s\n", Destination, Source);
return 0;
}
on the other hand, outputs
123 123
Please note, parameter order in strcpy is important (like in any other C function, after all).
|
|
|
|
|
Thanks, one of these days I'll hope to learn.
I did change the initialization and it did the trick with the exception of Result.
Can you explain to me why Result needs to be declared this way?
I appreciate all you guys time and I am sorry about the poor choice of variable names I did.
Thanks.
I am enclosing my full test code, just FYI , so do not pay much attention to it.
char *Result = {"0007890"};
char Destination[] = {"456"};
char Temp[] = {" "};
char Source[] = {"123"};
#ifdef DEBUG
lcd_i2c.clear();
//lcd_i2c.print("BuildAttribute...");
lcd_i2c.setCursor(0, 0);
lcd_i2c.print("Result ");
lcd_i2c.print(Result );
//for(;;);
lcd_i2c.setCursor(0, 1);
lcd_i2c.print("Source " );
lcd_i2c.print(Source );
//strcpy(PassedAttribute, "attribute");
lcd_i2c.setCursor(0, 2);
lcd_i2c.print("Dest " );
lcd_i2c.print(Destination );
lcd_i2c.setCursor(0, 3);
//lcd_i2c.print(Attribute);
//delay(2000);
//for(;;);
//return PassedAttribute;
#endif
//Result = strncpy(Source, "TESTDestination",2 );
printf("%s %s\n", Destination, Source);
Result = strcpy(Destination, Source);
printf("%s %s\n", Destination, Source);
//for (;;);
#ifdef DEBUG
lcd_i2c.clear();
//lcd_i2c.print("BuildAttribute...");
lcd_i2c.setCursor(0, 0);
lcd_i2c.print("Result ");
lcd_i2c.print(Result );
//for(;;);
lcd_i2c.setCursor(0, 1);
lcd_i2c.print("Source " );
lcd_i2c.print(Source );
//strcpy(PassedAttribute, "attribute");
lcd_i2c.setCursor(0, 2);
lcd_i2c.print("Dest " );
lcd_i2c.print(Destination );
lcd_i2c.setCursor(0, 3);
lcd_i2c.print("Temp ");
lcd_i2c.print(Temp);
//delay(2000);
for (;;);
//return PassedAttribute;
#endif
for(;;);
|
|
|
|
|
Result must be declared as pointer to character (char * ) because (as already explained by Richard) you are assigning it with strcpy return value. Please note such an assignment is useless since strcpy result is already in Destination array.
|
|
|
|
|
Thanks.
I know the results are already known, but I was just using it to learn about the strcpy.
|
|
|
|
|
This should exactly work:
#include <stdio.h>
#include <string.h>
int main(void)
{
char *Result = "Result init ";
char Source[256] = "123";
char Destination[256] = "456";
Result = strcpy(Source, Destination);
printf("Result = %s\nSource = %s\nDestination = %s\n", Result, Source, Destination);
return 0;
}
|
|
|
|
|
Hi,
I have build a VC++ dll project that build fine in visual studio environment.
I need to deploy that project to a server, which does not have VS2012, so I wrote a batch file to execute the build tasks using
Msbuild.
Please look at the code below:
@ECHO OFF
for /f "tokens=2*" %%A in ('REG QUERY "HKLM\SOFTWARE\Microsoft\MSBuild\ToolsVersions\4.0" /v MSBuildToolsPath') DO (
set MSBuildToolsPathVS=%%B)
@ECHO "MSbuildexePath= " %MSBuildToolsPathVS%
set SolFilePath="C:\C++\DllCreateProj\DllCreateProj.vcxproj"
@ECHO "SolFilePath= "%SolFilePath%
cd %MSBuildToolsPathVS%
msbuild.exe %SolFilePath% /p:configuration=release;Platform=x64
The project build fine,except the following warning:
Quote: Creating "x64\release\DllCreateproj.unsucessfullbuild" because "alwayscreate" was specified
I had this message before for the same project at different location, so I copied solution, and rebuilt it, and there is no such message in VS IDE but msbuild gives this message
I modify the script to log the results as:
@ECHO OFF
for /f "tokens=2*" %%A in (
set MSBuildToolsPathVS=%%B)
@ECHO "MSbuildexePath= " %MSBuildToolsPathVS%
set SolFilePath="C:\C++\DllCreateProj\DllCreateSol.sln"
@ECHO "SolFilePath= "%SolFilePath%
set MSBuildLogPath="C:\C++\DllCreateProj\Buildlog.txt"
@ECHO "MSBuildLogPath= "%MSBuildLogPath%
cd %MSBuildToolsPathVS%
msbuild.exe %SolFilePath% /p:configuration=release;Platform=x64^
/fileLoggerParameters:LogFile=%MSBuildLogPath%;Verbosity=detailed;
/t:clean;rebuild
Now, the paths print out correctly and I have verified that.
However, "Buildlog.txt" is not created and also, after the "build Suceeded" message, I get following message twice
Quote: The filename,directory name ,or volume label syntax is incorrect.
So, I have following questions:
1) How do I solve the warning for "unsucessfulbuild" ?
2) How do I specify path for log file correctly.
3) Why do I get the "volume label incorrect " message, and is there a way to get more information about that?
4) Given path to msbuild that I have extracted, I need to modify "SolFilePath" and "msBuildLogPath" as being relative to ,say release path Quote: C:\C++\DllCreateProj\x64\Release "
Any help with that?
Thanks
|
|
|
|
|
Hi Guys!,
I'm getting a runtime error, and compiler is pointing at "free.c", shown below:
void __cdecl _free_base (void * pBlock)
{
int retval = 0;
if (pBlock == NULL)
return;
RTCCALLBACK(_RTC_Free_hook, (pBlock, 0));
retval = HeapFree(_crtheap, 0, pBlock);
if (retval == 0)
{
errno = _get_errno_from_oserr(GetLastError());
}
}
I couldn't troubleshoot where exactly is the issue..
It would be of great help if you can give some suggestions..
Thanks!
|
|
|
|
|
 This is my main program..I'm sorry to post whole bunch of code here!, but I'm wondering this may helps someone can pointout the issue
int main(int argc, char* argv[])
{
<pre>
char fname[30];
char fpath[100]; char fpath1[100]; char fpath2[100];
memset(boolArray,0,sizeof(bool)*arraySize);
bool *pBool =boolArray + 10;
srand( (unsigned)time(NULL));
int rIndex2;
for(int i=0;i<10;++i){
memset(pBool,1,sizeof(bool)*12);
int rIndex = rand()%12;
pBool[rIndex]=0;
do{
rIndex2 = rand()%12;
pBool[rIndex2]=0;
} while (rIndex2==rIndex);
pBool +=12;
}
for (int j=0; j<NTRIALS;j++) printf("%d\t", boolArray[j]);
printf ("\n");
printf ("-----------------------------------\n");
printf ("-----------------------------------\n");
printf ("\n\n");
printf ("File name for logging:");
scanf("%s",fname);
printf ("\n\n");
printf ("[x] - Exit application\n");
printf ("\n\n");
resourceRoot = string(argv[0]).substr(0,string(argv[0]).find_last_of("/\\")+1);
sprintf(fpath,"%s\\%s%0d.dat",rootdir,fname);
pFile = fopen (fpath, "wb");
sprintf(fpath1,"%s\\%s%0d.pro",rootdir,fname);
pFile1 = fopen (fpath1, "w");
sprintf(fpath2,"%s\\%s%0d.arr",rootdir,fname);
pFile2 = fopen(fpath2,"w");
for (int j=0; j<NTRIALS;j++) fprintf(pFile2, "%d\t",boolArray[j]);
handler = new cHapticDeviceHandler();
numHapticDevices = handler->getNumDevices();
numHapticDevices = cMin(numHapticDevices, MAX_DEVICES);
int screenW = glutGet(GLUT_SCREEN_WIDTH);
int screenH = glutGet(GLUT_SCREEN_HEIGHT);
int windowPosX1 = 0;
int windowPosY1 = 0;
int windowPosX2 = WINDOW_SIZE_W;
int windowPosY2 = 0;
glutInitDisplayMode(GLUT_RGB | GLUT_DOUBLE);
glutInitWindowSize (WINDOW_SIZE_W/2, WINDOW_SIZE_H/2);
mytimer.start(true); statetimer.start(true);
int i = 0;
while (i < numHapticDevices){
cGenericHapticDevice* newHapticDevice;
handler->getDevice(newHapticDevice, i);
printf("index number of device-> %d \n\n", i);
newHapticDevice->open();
newHapticDevice->initialize();
hapticDevices[i] = newHapticDevice;
cHapticDeviceInfo info = newHapticDevice->getSpecifications();
CreateWorld(world[i],camera[i],cursors[i],starts[i],targets[i],via[i],labels[i],scorelabels[i]
,forcevectors[i],poslabels[i], waitlabels[i]);
char str[30];
sprintf(str,"ROBOT %d",i);
window[i] = glutCreateWindow (str);
if (i==0) {
glutInitWindowPosition (windowPosX1, windowPosY1);
glutDisplayFunc(updateGraphics1);
}
else {
glutInitWindowPosition (windowPosX2, windowPosY2);
glutDisplayFunc(updateGraphics2);
}
glutReshapeFunc(reshape);
glutKeyboardFunc(keySelect);
i++;
}
via[0]->setPos(cVector3d( 0, 0.02, 0.03));
via[1]->setPos(cVector3d( 0, -0.02, -0.03));
simulationRunning = true;
cThread* hapticsThread = new cThread();
hapticsThread->set(updateHaptics, CHAI_THREAD_PRIORITY_HAPTICS);
cThread* loggingThread = new cThread();
loggingThread->set(updateLogging, CHAI_THREAD_PRIORITY_HAPTICS);
cThread* IOThread = new cThread();
IOThread->set(IOflush, CHAI_THREAD_PRIORITY_GRAPHICS);
glutMainLoop();
mytimer.stop();
statetimer.stop();
fclose(pFile);
fclose(pFile1);
fclose(pFile2);
close();
return (0);
}
|
|
|
|
|
Why not remove everything that is not related to the problem, as it is just noise. Then we can look at what's left and make suggestions.
"One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"You can easily judge the character of a man by how he treats those who can do nothing for him." - James D. Miles
|
|
|
|
|
This sort of error is caused by code overwriting the beginning (or end) of a heap block, and corrupting the heap control structures. One way to find such a problem is to wrap each of your blocks in a "signature" (see below), and periodically check that the "signature" is valid.
Original structure:
class foo
{
int baz;
char bar;
};
New structure:
class foo
{
long sig1;
int baz;
char bar;
long sig2;
};
Note that you can use the constructor to initialize the signature, and the destructor to check whether the signatures have changed. Your methods can check the validity of the signatures before performing any operations. You can also modify the signatures in the destructor, in order to catch accesses to freed memory.
A library of this sort is a basic tool for any C++ programmer. If you don't have such a library in your toolbox - write one!
If you have an important point to make, don't try to be subtle or clever. Use a pile driver. Hit the point once. Then come back and hit it again. Then hit it a third time - a tremendous whack.
--Winston Churchill
|
|
|
|
|
This is usually sourced by writing to an array with an out of bound index.
So you must check all your allocated arrays (which may include arrays that are allocated by library functions). Starting with your own ones, search for any call to new as array; e.g.:
type arrayName = new type[size];
Then for each array check the accesses for invalid indexes (index < 0 or >= size). You can do this manually by reading your source or by guarding all accesses with assertions in debug builds:
assert(index > 0 && index < arrayNameSize);
arrayName[index] = someValue;
Your posted code does not contain the allocation of most. So it can't be checked by us. But there are many candidates: boolArray and all arrays used inside the 'while (i < numHapticDevices)' loop (e.g. hapticDevices ).
|
|
|
|
|
This seems very silly and basic -
The function description is pretty clear
Convert an unsigned long integer into a string, using a given base
So why I keep printing numbers and NOT ASCII letter?
I need a pointer to string - not numeric value.
And I am sorry to ask such stupid question.
Cheers
Vaclav
<pre>
int base = 10;
long value = 73;
char buffer[33];
unsigned long RANDOM = random(65, 79);
for( base = 2; base <= 16; base = base + 2 ) {
printf( "%2d %s\n", base,
ultoa( RANDOM, buffer, base ) );
}</pre>
|
|
|
|
|
I suppose, you know that
printf("%d\n", ul);
and
printf("%s\n", ultoa(ul, buffer, 10));
should produce the same output.
|
|
|
|
|