Cut down on compiler warnings
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Cuneiform for Linux |
New
|
Undecided
|
Unassigned |
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.
Jussi Pakkanen (jpakkane) wrote : | #1 |
Yury V. Zaytsev (zyv) wrote : | #2 |
Jussi Pakkanen (jpakkane) wrote : | #3 |
- msvc_fixes.patch Edit (37.7 KiB, text/plain)
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) wrote : | #4 |
- make-error.log Edit (5.3 KiB, text/plain)
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) wrote : | #5 |
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) wrote : Re: [Bug 314048] Re: Cut down on compiler warnings | #6 |
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:/
> 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) wrote : | #7 |
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) wrote : | #8 |
- Stevens patch breaks things for GCC Edit (65.8 KiB, text/plain)
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) wrote : | #9 |
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_
#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) wrote : | #10 |
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_
>
> #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:/
> 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) wrote : | #11 |
The cttypes.h thing was already discussed on the mailing list.
Yury V. Zaytsev (zyv) wrote : | #12 |
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) wrote : | #13 |
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:/
> 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) wrote : Invitation to connect on LinkedIn | #14 |
LinkedIn
------------
Bug,
I'd like to add you to my professional network on LinkedIn.
- Steven
Learn more:
https:/
-------
What is LinkedIn and why should you join?
http://
------
(c) 2009, LinkedIn Corporation
Jussi Pakkanen (jpakkane) wrote : | #15 |
Let this be a lesson to you all: don't automatically send email to every person in your address book.
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?