Cut down on compiler warnings

[Jussi Pakkanen]

The code currently emits quite a lot of compiler warnings. Getting rid of these would be beneficial

Attached is a patch from Steven Van Ingelgem that removes a lot of MSVC warnings. Unfortunately it breaks recognition completely on OS X.

[Yury V. Zaytsev (zyv)]

OMG, I'm buried with the exams, but that definitively looks interesting. Kudos for proper warning supression for MSVC (a concern I raised earlier). Does this stuff still work on Windoze or it also breaks?

[Jussi Pakkanen (jpakkane)]

Here is a patch from Dmitry Polevoy that also fixes stuff on MSVC (including MSVC 6). They probably overlap in some areas.

[Yury V. Zaytsev (zyv)]

1) This patch against the latest trunk breaks for me on Ubuntu 8.04 / GCC (see log attached).

2) This patch not only fixes MSVC compiler warnings but also implements batch mode processing for the console cuneiform client. I think this has to be split in two parts as those issues are completely unrelated.

[Yury V. Zaytsev (zyv)]

It seems that the reason for those errors are statements like this (example taken from cttypes.h):

// FIXME: MSVC++ 6

#if _MSC_VER > 1200

typedef uint16_t WORD;

#endif // _MSC_VER > 1200

WORD is not defined in GCC, but obviously does not get defined because _MSC_VER is not defined as well. I am to lame to propose a viable solution. Is it possible to convert those statements in the following form:

#if (_MSC_VER > 1200) && IS_GCC

where IS_GCC would be defined if the compiler is GCC?

[Steven Van Ingelgem (steven-vaningelgem-be)]

Hi Yury,

I don't think I ever do a "> 1200" because that's bad coding. I generally
include more files.
What I should do is start from my mega-patch and check why it breaks things.
That shouldn't be too difficult I suppose ;-).

Grtz,
Steven

2009/1/12 Yury V. Zaytsev <email address hidden>

> It seems that the reason for those errors are statements like this
> (example taken from cttypes.h):
>
> // FIXME: MSVC++ 6
>
> #if _MSC_VER > 1200
>
> typedef uint16_t WORD;
>
> #endif // _MSC_VER > 1200
>
>
> WORD is not defined in GCC, but obviously does not get defined because
> _MSC_VER is not defined as well. I am to lame to propose a viable solution.
> Is it possible to convert those statements in the following form:
>
> #if (_MSC_VER > 1200) && IS_GCC
>
> where IS_GCC would be defined if the compiler is GCC?
>
> --
> Cut down on compiler warnings
> https://bugs.launchpad.net/bugs/314048
> You received this bug notification because you are a member of Cuneiform
> Linux, which is the registrant for Cuneiform for Linux.
>
> Status in Linux port of Cuneiform: New
>
> Bug description:
> The code currently emits quite a lot of compiler warnings. Getting rid of
> these would be beneficial
>
> Attached is a patch from Steven Van Ingelgem that removes a lot of MSVC
> warnings. Unfortunately it breaks recognition completely on OS X.
>

[Yury V. Zaytsev (zyv)]

Hey, Steven,

On Mon, 2009-01-12 at 10:36 +0000, Steven Van Ingelgem wrote:

> I don't think I ever do a "> 1200" because that's bad coding. I
> generally include more files.

Sorry for the confusion, I was commenting on Dmitry's patch as they were
put into the same bug report by Jussi.

> What I should do is start from my mega-patch and check why it breaks things.
> That shouldn't be too difficult I suppose ;-).

Well, I think it is a good idea to fix your patch first, then tackle
with the rest of the issues cleaning up Dmitry's patch and finally
create a separate bug report for the "batch mode" part proposed by
Dmitry for inclusion.

I can try to build CF with YOUR patch on gcc version 4.2.4 (Ubuntu
4.2.4-1ubuntu3) and attach the logs / and test results if it would help
you with your mission :-)

--
Sincerely yours,
Yury V. Zaytsev

[Yury V. Zaytsev (zyv)]

Here we go.

I will try to come up with some points I didn't like in your patch later on (mind you, I am a newbie, so I might tag stuff as suspicious out of ignorance).

[Yury V. Zaytsev (zyv)]

Hi, Steven,

Please find my comments below:

* Generic note: isn't it better to add a "//FIXME: Unused vars, Steven"
when you declare unused vars to suppress the unused vars warning?

Maybe some time later these issues will be dealt with in a more correct
way, by just removing the 100% unused stuff from the function
declarations? If you just cut out the warnings no one will notice this
later.

* Same comment here (maybe better to tag with a FIXME):

=== modified file 'cuneiform_src/Kern/ced/sources/main/ced_func_old.cpp'

#ifdef _MSC_VER
# pragma warning ( disable: 4505 ) // unreferenced local function has
been removed
#endif // _MSC_VER

* You added lots of C-style casts; well, adding implicit casts is a good
idea but please make sure it won't break on other platforms. E.g. I am
always afraid of those (*int) casts because it might have different size
on x64? If I am wrong, I apologize for the confusion.

* for ( ;; ) is great but in essence this is the same that while(TRUE).
Maybe the function implementation have to be rethought instead of just
suppressing it?

* I find it strange that replacing += with stuff = stuff + stuff fixes
things. Maybe this has to be investigated further (ctb_pack.c etc.)?

* ccom.c diff looks weird?! It replaces the whole file. Did you really
change every single line? Probably this is because of the \r\n line
endings. Let us stick to the uniform line endings.

* ccom.c at the very end:

#ifdef _MSC_VER
# pragma warning (disable: 4055) // from data pointer 'void *' to
function pointer 'my_alloc_type'
#endif // _MSC_VER

Don't you think we'd better use reinterpret_cast<> ?

My argument in favor of this is that these casts can be easily grepped
as opposed to the C-style casts which very are hard to notice. Even
though Jussi might be right about static_cast<> vs (int) (it does not
make sense to use them because they are harmless anyway, he says), but I
feel that reinterpret_casts are very dangerous and well deserve it.

* Same for the code below:

#ifdef _MSC_VER
# pragma warning( disable: 4054 ) // 'type cast' : from function
pointer '...' to data pointer '...'
# pragma warning( disable: 4055 ) // 'type cast' : from data pointer
'...' to function pointer '...'
#endif

* crling.h, loc.c - line endings?

Hope that helps and thank you for your efforts!

--
Sincerely yours,
Yury V. Zaytsev

[Steven Van Ingelgem (steven-vaningelgem-be)]

Well Yury, I basically gave up because the code was - lets face it - a true
mess.

At least on VC++ 2005, if you use the "+=" operator it is returning an
"int", which is sometimes assigned to a short, so than you get a warning.

A reinterpret_cast only exists in C++, not in C. ccom.c is a C-file, not a
C++ and thus cannot be used.

Greetings,
Steven

2009/1/12 Yury V. Zaytsev <email address hidden>

> Hi, Steven,
>
> Please find my comments below:
>
> * Generic note: isn't it better to add a "//FIXME: Unused vars, Steven"
> when you declare unused vars to suppress the unused vars warning?
>
> Maybe some time later these issues will be dealt with in a more correct
> way, by just removing the 100% unused stuff from the function
> declarations? If you just cut out the warnings no one will notice this
> later.
>
> * Same comment here (maybe better to tag with a FIXME):
>
> === modified file 'cuneiform_src/Kern/ced/sources/main/ced_func_old.cpp'
>
> #ifdef _MSC_VER
> # pragma warning ( disable: 4505 ) // unreferenced local function has
> been removed
> #endif // _MSC_VER
>
> * You added lots of C-style casts; well, adding implicit casts is a good
> idea but please make sure it won't break on other platforms. E.g. I am
> always afraid of those (*int) casts because it might have different size
> on x64? If I am wrong, I apologize for the confusion.
>
> * for ( ;; ) is great but in essence this is the same that while(TRUE).
> Maybe the function implementation have to be rethought instead of just
> suppressing it?
>
> * I find it strange that replacing += with stuff = stuff + stuff fixes
> things. Maybe this has to be investigated further (ctb_pack.c etc.)?
>
> * ccom.c diff looks weird?! It replaces the whole file. Did you really
> change every single line? Probably this is because of the \r\n line
> endings. Let us stick to the uniform line endings.
>
> * ccom.c at the very end:
>
> #ifdef _MSC_VER
> # pragma warning (disable: 4055) // from data pointer 'void *' to
> function pointer 'my_alloc_type'
> #endif // _MSC_VER
>
> Don't you think we'd better use reinterpret_cast<> ?
>
> My argument in favor of this is that these casts can be easily grepped
> as opposed to the C-style casts which very are hard to notice. Even
> though Jussi might be right about static_cast<> vs (int) (it does not
> make sense to use them because they are harmless anyway, he says), but I
> feel that reinterpret_casts are very dangerous and well deserve it.
>
> * Same for the code below:
>
> #ifdef _MSC_VER
> # pragma warning( disable: 4054 ) // 'type cast' : from function
> pointer '...' to data pointer '...'
> # pragma warning( disable: 4055 ) // 'type cast' : from data pointer
> '...' to function pointer '...'
> #endif
>
> * crling.h, loc.c - line endings?
>
> Hope that helps and thank you for your efforts!
>
> --
> Sincerely yours,
> Yury V. Zaytsev
>
> --
> Cut down on compiler warnings
> https://bugs.launchpad.net/bugs/314048
> You received this bug notification because you are a member of Cuneiform
> Linux, which is the registrant for Cuneiform for Linux.
>
> Status in Linux port of Cuneiform: New
>
> Bug description:
> The code currently emits quite a lot of compile...

[Jussi Pakkanen (jpakkane)]

The cttypes.h thing was already discussed on the mailing list.

https://lists.launchpad.net/cuneiform/msg00157.html

[Yury V. Zaytsev (zyv)]

Hey, Steven,

So you are not going to look into this further :-( ? That is sad. I didn't mean to discourage you, but not being a programmer myself just to provide some useful feedback.

Cheers,
--Yury.

[Steven Van Ingelgem (steven-vaningelgem-be)]

Not at this point in time no. You didn't discourage me at all. In fact you
gave me a little incentive.
But not enough to battle this kind of code. It's really ugly, and need to be
completely rewritten in a much more clean and consistent way.
There are such horrors inside there you don't even want to go into. And I
did battle already some nasty code in my life :-).

So as for now I think the best chance is to look what's going wrong in the
initialization procedure. As basically what I did in my patch is casting,
and rewriting of "+=" and "-=" and so on...

2009/1/12 Yury V. Zaytsev <email address hidden>

> Hey, Steven,
>
> So you are not going to look into this further :-( ? That is sad. I
> didn't mean to discourage you, but not being a programmer myself just to
> provide some useful feedback.
>
> Cheers,
> --Yury.
>
> --
> Cut down on compiler warnings
> https://bugs.launchpad.net/bugs/314048
> You received this bug notification because you are a member of Cuneiform
> Linux, which is the registrant for Cuneiform for Linux.
>
> Status in Linux port of Cuneiform: New
>
> Bug description:
> The code currently emits quite a lot of compiler warnings. Getting rid of
> these would be beneficial
>
> Attached is a patch from Steven Van Ingelgem that removes a lot of MSVC
> warnings. Unfortunately it breaks recognition completely on OS X.
>

[Steven Van Ingelgem (steven-vaningelgem-be)]

LinkedIn
------------

Bug,

I'd like to add you to my professional network on LinkedIn.

- Steven

Learn more:
https://www.linkedin.com/e/isd/538635200/YyFM7GsS/

------------------------------------------

What is LinkedIn and why should you join?
http://learn.linkedin.com/what-is-linkedin

------
(c) 2009, LinkedIn Corporation

[Jussi Pakkanen (jpakkane)]

Let this be a lesson to you all: don't automatically send email to every person in your address book.