Lighta Posted December 2, 2012 Group: Members Topic Count: 16 Topics Per Day: 0.00 Content Count: 737 Reputation: 216 Joined: 11/29/11 Last Seen: December 20, 2020 Share Posted December 2, 2012 (edited) So this is kinda old stuff but I think we should take more in accounts warnings and if possible fix them. Currently we only have fews build who throwing out warnings : (http://build.rathena...ings (40)]NMAKE[/url]) cause other one are either set as silent or don't got a warnings level sufficient. I'm suggesting to change visual warnings rom lvl 3 to lvl 4 and modify configure to remove -Wno parenthese, -Wno unused -Wno-sign-compare -Wno-pointer-sign. For configure since we trying to optimise and make the code stronger those silent warnings serve againt our purpose, Why do we need to silent parenthese, bettter to let us know if a condition is dubious : if(a && b || c) => do you mean ((a && || c) or (a && (b || c)). unused is trivial and worse is done in Vcc but not in Gcc cause we tell him to shut us, what the logic in that ? The only one I believe should stay is -Wno switch cause we using lot of switch and enum, and he will complaint to not have them all, well we could always put a default path for all of them. If there still some warnings and you feel we should silent them, let at least do it if degug mode is off (--enable debug) so we can easely take a look at it later. Of course I also believe buildbot should have debug mode on for compile so we know what missing. Configure diff Vcc Files If you doing this you'll see those kind of warnings : http://upaste.me/e1dc170363199020 So quick study of them we assigning to smaller type a bigger type like : short a = int b, so ofc only half of b will be assign. Signed to unsigned or unsigned to signed short a = unsigned short b; unsigned short a = short a; So ofc after SHRT_MAX will have some issue of sign nonstandard extension used : bit field types char a:1; unsigned short a:1; All useless we said we wanted only 1 bit assign for this type, putting short or char is useless as we still only gonna have 1bit This only bring confusion as expected type, real declaration is : int a:x (x = number of bit) etc... So just changing those ain't hat hard and will put the code more coherent, they were mostly done by optimisation attempt, like we change mapid to short but not in function call causing those issues or trought scanf... This will also optimise the code cause we will reduce in most case type so less ram reserved. I already starting doing the change, ain't that easy to explain rule change but it's basically : -1) keep struct assign in type if optimise; -2) harmonize other struct we have the same index in there. -3) Update local variable for correct match -4) Update function parameter call -5) cast Here what I've done for the moment : ra_warns2.diff So tell me what u think about it Edited December 4, 2012 by Lighta updated diff 2 Quote Link to comment Share on other sites More sharing options...
xazax Posted December 3, 2012 Group: Members Topic Count: 11 Topics Per Day: 0.00 Content Count: 427 Reputation: 123 Joined: 11/17/11 Last Seen: December 31, 2022 Share Posted December 3, 2012 Maybe it is just me, but I prefer to only do one specific refactoring with one commit. For example to turn on one warning in gcc and fix those warnings with the same commit may be a bit easier to track in the future than huge changesets. For the same reason it may be better to first enable several gcc warning and only set w4 warning level in visual studio after the warnings in gcc are fixed. 1 Quote Link to comment Share on other sites More sharing options...
Lighta Posted December 3, 2012 Group: Members Topic Count: 16 Topics Per Day: 0.00 Content Count: 737 Reputation: 216 Joined: 11/29/11 Last Seen: December 20, 2020 Author Share Posted December 3, 2012 If I do turn them on without fix this would generate lot of warnings for people. Well I can put tjem under debug mode only for gcc but I've got no clue how to set that for VCC. I'd also prefer to not have a huge changeset but this may be complicate for this one, well I sure can do 1 for refactoring then the next one following for type mistmatch fix wich would reduce the amount but the one for refactoring people may not like it since it won't bring any interesting change except that it's easier for us to follow those type. (eg : one refactoring in here is skill_num, skillid, skillnum even sometime skill => skillid) They all represent skill_id but was named differently, not such a big deal but it's easier to track skill_id with the same name then search for those other type, after that I'll put them all to same type with one big change. (currently some are ushort, uint, int, ...) wich is not safe for overflow case could be optimize easily. Quote Link to comment Share on other sites More sharing options...
xazax Posted December 4, 2012 Group: Members Topic Count: 11 Topics Per Day: 0.00 Content Count: 427 Reputation: 123 Joined: 11/17/11 Last Seen: December 31, 2022 Share Posted December 4, 2012 Well, HUGE +1 to have uniform names for variables. Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.