Re: [Warzone-dev] g++ fixes again
On 5/25/07, Dennis Schridde [EMAIL PROTECTED] wrote: Am Freitag, 25. Mai 2007 schrieb Giel van Schijndel: Patch is attached for review. I haven't looked into a typedef solution to eliminate the struct keyword yet, so if you happen to know one right away, please tell me. Sorry forgot to mention, with review, I meant whether no-one objects this approach. Especially the resulting consequence of needing to specify struct for each declaration additionally. Afaik you don't need to mention the struct in the C file. Using the typedef should be ok. For what it is worth, I dislike attempts to hide pointer references and struct or enum keywords from variable declarations using typedefs. This makes it harder for the programmer to know what he is dealing with (is this dereferenced? can I pass it by value? what happens if I assign one to the other? can I compare them?). - Per ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
Per Inge Mathisen schreef: On 5/25/07, Dennis Schridde [EMAIL PROTECTED] wrote: Am Freitag, 25. Mai 2007 schrieb Giel van Schijndel: Patch is attached for review. I haven't looked into a typedef solution to eliminate the struct keyword yet, so if you happen to know one right away, please tell me. Sorry forgot to mention, with review, I meant whether no-one objects this approach. Especially the resulting consequence of needing to specify struct for each declaration additionally. Afaik you don't need to mention the struct in the C file. Using the typedef should be ok. For what it is worth, I dislike attempts to hide pointer references and struct or enum keywords from variable declarations using typedefs. This makes it harder for the programmer to know what he is dealing with (is this dereferenced? can I pass it by value? what happens if I assign one to the other? can I compare them?). Can I interpret this as that you actually prefer to see the struct keyword in declarations ? I do agree btw that this explicit declaration as a struct-type makes code more readable. -- Giel signature.asc Description: OpenPGP digital signature ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
On 5/25/07, Giel van Schijndel [EMAIL PROTECTED] wrote: Can I interpret this as that you actually prefer to see the struct keyword in declarations ? Yes. - Per ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
Christian Ohm schreef:
On Wednesday, 23 May 2007 at 22:46, Giel van Schijndel wrote:
Christian Ohm schreef:
Some g++ fixes again. new is a reserved keyword, a few casts, and I've
moved OggVorbisDecoderState into oggvorbis.h, there was a linker error
when it was typedeffed to void in some cases. Don't know why it was done
that way, still works for me now. I've removed an #ifdef WZ_NOOGG, but
./configure --disable-ogg was broken before anyway.
That double typedef was done to encapsulate the OggVorbis decoding code,
compare it to a C++ pimpl if you wish. The linker error is caused by
C++'s function overloading, so the correct fix is not to expose details,
but disabling function overloading for that function: extern C.
Hm, in C this looks quite inelegant to me; just because the struct is
exposed to the outside doesn't mean you have to access it directly. In
C++ the whole class is exposed as well, you just have the access control
via private (unless you just use pointers to the class; I've thought
about that as well but was too lazy to try to rewrite the code that
way).
I have to agree that it is quite inelegant. Although my approach isn't
similar to C++'s private keyword, it's more equal to the Pimpl idiom. (
http://www.gamedev.net/reference/articles/article1794.asp )
However the void* pointer indeed isn't nice. Therefore I've now taken a
new approach to this: forward declaration of the struct (which is enough
to create a pointer to it). It does, however, require the addition of
the keyword 'struct' in front of every declaration of
OggVorbisDecoderState or pointers to it.
Patch is attached for review. I haven't looked into a typedef solution
to eliminate the struct keyword yet, so if you happen to know one right
away, please tell me.
--
Giel
Index: lib/sound/cdaudio.c
===
--- lib/sound/cdaudio.c (revision 1337)
+++ lib/sound/cdaudio.c (working copy)
@@ -52,7 +52,7 @@
static ALuint music_buffers[NB_BUFFERS];
static ALuint music_source;
-OggVorbisDecoderState* decoder = NULL;
+struct OggVorbisDecoderState* decoder = NULL;
static inline unsigned int numProcessedBuffers(void)
{
Index: lib/sound/oggvorbis.c
===
--- lib/sound/oggvorbis.c (revision 1337)
+++ lib/sound/oggvorbis.c (working copy)
@@ -30,7 +30,9 @@
#define OGG_ENDIAN 0
#endif
-typedef struct
+#include oggvorbis.h
+
+struct OggVorbisDecoderState
{
// Internal identifier towards PhysicsFS
PHYSFS_file* fileHandle;
@@ -43,20 +45,17 @@
// Internal meta data
vorbis_info* VorbisInfo;
-} OggVorbisDecoderState;
+};
-#define _LIBSOUND_OGGVORBIS_C_
-#include oggvorbis.h
-
static size_t wz_oggVorbis_read(void *ptr, size_t size, size_t nmemb, void
*datasource)
{
-PHYSFS_file* fileHandle = ((OggVorbisDecoderState*)datasource)-fileHandle;
+PHYSFS_file* fileHandle = ((struct
OggVorbisDecoderState*)datasource)-fileHandle;
return PHYSFS_read(fileHandle, ptr, 1, size*nmemb);
}
static int wz_oggVorbis_seek(void *datasource, ogg_int64_t offset, int whence)
{
-PHYSFS_file* fileHandle = ((OggVorbisDecoderState*)datasource)-fileHandle;
-BOOL allowSeeking = ((OggVorbisDecoderState*)datasource)-allowSeeking;
+PHYSFS_file* fileHandle = ((struct
OggVorbisDecoderState*)datasource)-fileHandle;
+BOOL allowSeeking = ((struct
OggVorbisDecoderState*)datasource)-allowSeeking;
int curPos, fileSize, newPos;
@@ -101,7 +100,7 @@
}
static long wz_oggVorbis_tell(void *datasource) {
-PHYSFS_file* fileHandle = ((OggVorbisDecoderState*)datasource)-fileHandle;
+PHYSFS_file* fileHandle = ((struct
OggVorbisDecoderState*)datasource)-fileHandle;
return PHYSFS_tell(fileHandle);
}
@@ -112,11 +111,11 @@
wz_oggVorbis_tell
};
-OggVorbisDecoderState* sound_CreateOggVorbisDecoder(PHYSFS_file*
PHYSFS_fileHandle, BOOL allowSeeking)
+struct OggVorbisDecoderState* sound_CreateOggVorbisDecoder(PHYSFS_file*
PHYSFS_fileHandle, BOOL allowSeeking)
{
int error;
- OggVorbisDecoderState* decoder = malloc(sizeof(OggVorbisDecoderState));
+ struct OggVorbisDecoderState* decoder = malloc(sizeof(struct
OggVorbisDecoderState));
if (decoder == NULL)
{
debug(LOG_ERROR, sound_CreateOggVorbisDecoder: couldn't
allocate memory\n);
@@ -140,7 +139,7 @@
return decoder;
}
-void sound_DestroyOggVorbisDecoder(OggVorbisDecoderState* decoder)
+void sound_DestroyOggVorbisDecoder(struct OggVorbisDecoderState* decoder)
{
// Close the OggVorbis decoding stream
ov_clear(decoder-oggVorbis_stream);
@@ -148,27 +147,27 @@
free(decoder);
}
-static inline unsigned int getSampleCount(OggVorbisDecoderState* decoder)
+static inline unsigned int getSampleCount(struct OggVorbisDecoderState*
decoder)
{
int numSamples =
Re: [Warzone-dev] g++ fixes again
On 5/23/07, Christian Ohm [EMAIL PROTECTED] wrote: Some g++ fixes again. Most of these are good, but do we need fixes of this kind?: - buffer = malloc(bufferSize + sizeof(soundDataBuffer)); + buffer = (soundDataBuffer*) malloc(bufferSize + sizeof(soundDataBuffer)); Please just use -fpermissive, or something, and don't clutter the code with useless (and hard to maintain properly) casts like this. It is not good C coding practice. - Per ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
Christian Ohm schreef: Some g++ fixes again. new is a reserved keyword, a few casts, and I've moved OggVorbisDecoderState into oggvorbis.h, there was a linker error when it was typedeffed to void in some cases. Don't know why it was done that way, still works for me now. I've removed an #ifdef WZ_NOOGG, but ./configure --disable-ogg was broken before anyway. That double typedef was done to encapsulate the OggVorbis decoding code, compare it to a C++ pimpl if you wish. The linker error is caused by C++'s function overloading, so the correct fix is not to expose details, but disabling function overloading for that function: extern C. -- Giel signature.asc Description: OpenPGP digital signature ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
Am Mittwoch, 23. Mai 2007 schrieb Per Inge Mathisen: On 5/23/07, Christian Ohm [EMAIL PROTECTED] wrote: Some g++ fixes again. Most of these are good, but do we need fixes of this kind?: - buffer = malloc(bufferSize + sizeof(soundDataBuffer)); + buffer = (soundDataBuffer*) malloc(bufferSize + sizeof(soundDataBuffer)); Please just use -fpermissive, or something, and don't clutter the code with useless (and hard to maintain properly) casts like this. It is not good C coding practice. Question: I've come along things like these before, too. The problem was that you need to match the sizeof() with the type of var you want to assign the memory to. I decided in that case, that it is good to explicitly cast, since then you will immediately notice that you forgot to change that assignment/allocation after changing the type of the var. So I found it to be useful for mallocs at least. What do you think about this? Are there any problems with an explicit cast? Why should I not cast? --Dennis signature.asc Description: This is a digitally signed message part. ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
Giel van Schijndel schreef: Christian Ohm schreef: Some g++ fixes again. new is a reserved keyword, a few casts, and I've moved OggVorbisDecoderState into oggvorbis.h, there was a linker error when it was typedeffed to void in some cases. Don't know why it was done that way, still works for me now. I've removed an #ifdef WZ_NOOGG, but ./configure --disable-ogg was broken before anyway. That double typedef was done to encapsulate the OggVorbis decoding code, compare it to a C++ pimpl if you wish. The linker error is caused by C++'s function overloading, so the correct fix is not to expose details, but disabling function overloading for that function: extern C Applied your fix for imdload.c (changed `newV` to `newVector` though). As for oggvorbis.[ch], I took the extern C approach. I left the casts from void* out since as Per pointed out it isn't proper C. Nor is it proper C++ either, since in C++ polymorphism should be used instead of casting from void* all over the place. In fact C++'s virtual functions + inheritance are meant to replace void* usage, as long as we're not using C++ however that approach won't be viable. -- Giel signature.asc Description: OpenPGP digital signature ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
On Wed, 23 May 2007 17:12:02 -0400 Per Inge Mathisen [EMAIL PROTECTED] wrote: On 5/23/07, Dennis Schridde [EMAIL PROTECTED] wrote: Most of these are good, but do we need fixes of this kind?: - buffer = malloc(bufferSize + sizeof(soundDataBuffer)); + buffer = (soundDataBuffer*) malloc(bufferSize + sizeof(soundDataBuffer)); ... Question: ... Why should I not cast? Casting is bad because it makes changing types later obnoxiously hard. It also clutters the code badly. Actually, the best way to do it is like this: buffer = malloc(bufferSize + sizeof(*buffer)); This way you do not explicitly mention the type, and if you later change it, it will not cause size or casting problems. - Per What is use of sizeof (*buffer) ? Padding? -- Looking for insurance? Compare and save 50% today. Click here. http://tagline.hushmail.com/fc/CAaCXv1QT6sib8saUWFKgBKPBhAepxgj/ ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
[EMAIL PROTECTED] schreef: On Wed, 23 May 2007 17:12:02 -0400 Per Inge Mathisen [EMAIL PROTECTED] wrote: On 5/23/07, Dennis Schridde [EMAIL PROTECTED] wrote: Most of these are good, but do we need fixes of this kind?: - buffer = malloc(bufferSize + sizeof(soundDataBuffer)); + buffer = (soundDataBuffer*) malloc(bufferSize + sizeof(soundDataBuffer)); ... Question: ... Why should I not cast? Casting is bad because it makes changing types later obnoxiously hard. It also clutters the code badly. Actually, the best way to do it is like this: buffer = malloc(bufferSize + sizeof(*buffer)); This way you do not explicitly mention the type, and if you later change it, it will not cause size or casting problems. What is use of sizeof (*buffer) ? Padding? actually they were talking about this change: - buffer = malloc(bufferSize + sizeof(soundDataBuffer)); + buffer = (soundDataBuffer*) malloc(bufferSize + sizeof(soundDataBuffer)); So sizeof(*buffer) == sizeof(soundDataBuffer). So it really isn't the sizeof that is for padding, but the bufferSize. Look into lib/sound/oggvorbis.c for more info. -- Giel signature.asc Description: OpenPGP digital signature ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
On Wednesday, 23 May 2007 at 22:42, Per Inge Mathisen wrote: On 5/23/07, Christian Ohm [EMAIL PROTECTED] wrote: Some g++ fixes again. Most of these are good, but do we need fixes of this kind?: - buffer = malloc(bufferSize + sizeof(soundDataBuffer)); + buffer = (soundDataBuffer*) malloc(bufferSize + sizeof(soundDataBuffer)); Please just use -fpermissive, or something, and don't clutter the code with useless (and hard to maintain properly) casts like this. It is not good C coding practice. Oh, sorry, either I forgot the -fpermissive when changing that, or mistook the warning message for an error. You're right, it's not needed. -- The covers of this book are too far apart. -- Book review by Ambrose Bierce. ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
Re: [Warzone-dev] g++ fixes again
On Wednesday, 23 May 2007 at 22:46, Giel van Schijndel wrote: Christian Ohm schreef: Some g++ fixes again. new is a reserved keyword, a few casts, and I've moved OggVorbisDecoderState into oggvorbis.h, there was a linker error when it was typedeffed to void in some cases. Don't know why it was done that way, still works for me now. I've removed an #ifdef WZ_NOOGG, but ./configure --disable-ogg was broken before anyway. That double typedef was done to encapsulate the OggVorbis decoding code, compare it to a C++ pimpl if you wish. The linker error is caused by C++'s function overloading, so the correct fix is not to expose details, but disabling function overloading for that function: extern C. Hm, in C this looks quite inelegant to me; just because the struct is exposed to the outside doesn't mean you have to access it directly. In C++ the whole class is exposed as well, you just have the access control via private (unless you just use pointers to the class; I've thought about that as well but was too lazy to try to rewrite the code that way). -- Some books are to be tasted, others to be swallowed, and some few to be chewed and digested. -- Francis Bacon [As anyone who has ever owned a puppy already knows. Ed.] ___ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
