Jump to content
The Dark Mod Forums

Search the Community

Showing results for tags 'compilation warning'.

  • Search By Tags

    Type tags separated by commas.
  • Search By Author

Content Type


Forums

  • General Discussion
    • News & Announcements
    • The Dark Mod
    • Fan Missions
    • Off-Topic
  • Feedback and Support
    • TDM Tech Support
    • DarkRadiant Feedback and Development
    • I want to Help
  • Editing and Design
    • TDM Editors Guild
    • Art Assets
    • Music & SFX

Find results in...

Find results that contain...


Date Created

  • Start

    End


Last Updated

  • Start

    End


Filter by number of...

Joined

  • Start

    End


Group


AIM


MSN


Website URL


ICQ


Yahoo


Jabber


Skype


Location


Interests

Found 2 results

  1. While analysing the warnings emitted by GCC 6, I was pointed to this piece of code in renderer/Model_lwo.cpp (comment added): int sgetI1( unsigned char **bp ) { int i; if ( flen == FLEN_ERROR ) return 0; i = **bp; if ( i > 127 ) i -= 256; flen += 1; *bp++; // <== warning: unused value return i; } short sgetI2( unsigned char **bp ) { short i; if ( flen == FLEN_ERROR ) return 0; memcpy( &i, *bp, 2 ); BigRevBytes( &i, 2, 1 ); flen += 2; *bp += 2; return i; } Starting from the second function, sgetI2(), it appears that it receives as argument a pointer to an unsigned character, passed by reference in C style (that becomes a pointer to the pointer).It copies two bytes in a short, swap them as needed (handling little/big endian, I suppose), then it makes the pointer *bp point after the copied data and returns the value read. There is a sgetI4() which does the same with 4 byte data. Back to the first quoted, I was assuming sgetI1() would do the same. GCC complains that the statement *bpp++ produces an unused value. It turns out, the postfix increment operator (a++) has the highest priority among C and C++ operators, and it gets executed before the dereference operator. That means that I would expect the code to operate like (*bp)++, equivalent to *bp += 1, similarly to sgetI2(), but I get instead a *(bp++). This means that the local variable bp (unsigned char**) is increased (while the unsigned char* pointer *bp is not), and then the value it was pointing to before the increase is taken and ignored. All in all, it means that when it's called as sgetI1(&ptrToBuffer), it returns the value at the current value of ptrToBuffer and, on the next call, sgetI1(&ptrToBuffer), it returns again the same value, since ptrToBuffer is not increased. The same construct appears in sgetU1() and in add_clip() on nclips (a simple pointer int*), where it might have more serious consequences: *nclips++; clip->index = *nclips; since the an index is assigned from the cell of memory next to nclips (nclips is first increased).I can't find any place where the former two are used, while add_clip() is pulled in from LoadLWO() in renderer/Model.cpp (at which point I stopped tracking). I attach a trivial patch that removes these warnings by just doing the thing that would be usually expected (increase the pointed value) (it also removes the other instance of it, not mentioned in this text). Edit: discovered that syntax highlight for C/C++ works with code type "auto", even if it does not show in the preview. UnusedValue.patch.txt
  2. This is another report originating from a GCC warning, in this case the comparison of two different enumerators. This works by converting each of the two enumerators into a numeric value, and comparing them instead. In general, an enumerator is better used in a way that does not depend from its value at all. If the value is needed, constants are generally better suited. The relevant one of the two warnings comes from idAF::Load() in game/AF.cpp (line 910), where a variable of enumerator type declAFConstraintType_t from framework/DeclAF.h: typedef enum { DECLAF_CONSTRAINT_INVALID, DECLAF_CONSTRAINT_FIXED, DECLAF_CONSTRAINT_BALLANDSOCKETJOINT, DECLAF_CONSTRAINT_UNIVERSALJOINT, DECLAF_CONSTRAINT_HINGE, DECLAF_CONSTRAINT_SLIDER, DECLAF_CONSTRAINT_SPRING } declAFConstraintType_t;is compared with a variable of enumerator type constraintType_t from game/physics/Physics_AF.h: typedef enum { CONSTRAINT_INVALID, CONSTRAINT_FIXED, CONSTRAINT_BALLANDSOCKETJOINT, CONSTRAINT_UNIVERSALJOINT, CONSTRAINT_HINGE, CONSTRAINT_HINGESTEERING, CONSTRAINT_SLIDER, CONSTRAINT_CYLINDRICALJOINT, CONSTRAINT_LINE, CONSTRAINT_PLANE, CONSTRAINT_SPRING, CONSTRAINT_CONTACT, CONSTRAINT_FRICTION, CONSTRAINT_CONELIMIT, CONSTRAINT_PYRAMIDLIMIT, CONSTRAINT_SUSPENSION } constraintType_t;The comparison of values from two distinct enumerator types should be avoided because it requires the enumerator modifications to be synchronised, but it's very tempting to add an element to one forgetting about the other. If this comparison is really required, there should be a single enumerator type. Judging from the enumerator names, this departure from synchronisation has already happened (I would except a DECLAF_CONSTRAINT_SLIDER to be equivalent to a CONSTRAINT_SLIDER, and instead it's compared to a CONSTRAINT_HINGESTEERING). The two definitions are in different parts of the code and do not apparently share headers. But more to the point, for what I know these numerical values might be serialised, and changing them would break saved data. My recommended suggestions would be: if possible, manage to have a single enumerator for both usesif not, abandon direct comparison and use a slower multiple-choice comparison function.I provide a patch implementing the latter suggestion, which is expected not to break any serialised data, by not changing the enumerator values. Note that this function relies on the heuristic logic of my brain to match the elements from the two enumerators. Somebody with some understanding should cross check it. The patch also addresses the other enumerator comparison, which is more subtle and is caused by the fact that enumerators defined in a template class yield one different enumerator type for each template instantiation. enumCompare.patch.txt
×
×
  • Create New...