Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tons of compiler warnings #44

Merged
merged 12 commits into from
May 24, 2016
Merged

Conversation

DanielGibson
Copy link

@DanielGibson DanielGibson commented Apr 23, 2016

only for the TSE build, I guess when building TFE there are some more
both for TSE and TFE builds

also includes a commit fixing the depth buffer problems from #37

@@ -0,0 +1,108 @@
/* A Bison parser, made by GNU Bison 3.0.2. */
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, Engine/Base/Parser.hpp wasn't supposed to be committed, will push a fixed (and rebased) version in minute

@DanielGibson DanielGibson force-pushed the misc-impr branch 2 times, most recently from 91fa4f8 to 9f851e6 Compare April 23, 2016 23:02
@DanielGibson
Copy link
Author

ok, removed accidentally committed files, should be fine now

@DanielGibson DanielGibson mentioned this pull request Apr 23, 2016
@@ -509,7 +509,7 @@ CBrushPolygon &CBrushPolygon::CopyPolygon(CBrushPolygon &bp)
bpo_boxBoundingBox=bp.bpo_boxBoundingBox;
bpo_pbscSector=bp.bpo_pbscSector;
bpo_rsOtherSideSectors.Clear();
bpo_lhShadingInfos;
//bpo_lhShadingInfos; // FIXME: DG: was this missing "= bp.bpo_lhShadingInfos" or something like that?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a copying function, bpo_lhShadingInfos = bp.bpo_lhShadingInfos; is very likely what was intended.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, /could/ be, but some other fields were cleared or set to NULL, so I don't know for sure.. and it seems to work like it is, so unless a bug appears due to that field not being copied, maybe it should just be left commented out?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you can not copy list heads. bpo_lhShadingInfos is a CListHead, isn't it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. thanks! :-)

@@ -246,7 +246,7 @@ void LastUnreadMessage(void)
// go to next/previous message
void PrevMessage(void)
{
if (_iActiveMessage<_acmMessages.Count()==0) {
if (_iActiveMessage < _acmMessages.Count()==0) { // FIXME: DG: what is this about?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be equivalent to if ( ! (_iActiveMessage < _acmMessages.Count()) ) which seems to make sense.. - any objections?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So written in a non-obfuscated way, that would be if (_iActiveMessage >= _acmMessages.Count()), right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yes.

My original confusion when adding the FIXME to the code came from assuming that it was supposed to mean if(_iActiveMessage < (_acmMessages.Count()==0)), but > has higher priority than == so that assumption was incorrect

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced it with if (_iActiveMessage >= _acmMessages.Count()) now.

@DanielGibson
Copy link
Author

hmm now github claims there are merge conflicts - will rebase to master in a minute

the builtins are only used when using GCC or clang, of course, otherwise
the usual shifting is done.

Them being inline functions instead of macros increases type safety
and gets rid of problems with signed shifts.

Changed two places in the code that swapped bytes in 32bit ints to use
BYTESWAP32_unsigned() instead - in case of PrepareTexture() this has
probably even fixed issues with signed shifts
Fixed -Wreorder warnings (hopefully the last?), also several potentially
uninitialized variables.

In SetupPixelFormat_OGL() Assert if gap_iDepthBits ends up being 0.

Small adjustments to cmake warning settings for gcc/clang
many unused functions and variables are now commented out

You'll still get tons of warnings, which should mostly fall in one of
the following categories:
1. Unnecessary variables or values generated from .es scripts
2. Pointers assigned to from functions with side-effects: DO NOT REMOVE!
   Like CEntity *penNew = CreateEntity_t(...); - even if penNew isn't
   used, CreateEntity() must be called there!
except for EntitiesMP/Fish.es which I'm not sure about, and in
Computer.cpp the weird "if (_iActiveMessage < _acmMessages.Count()==0)"
construct whichs intention I didn't fully grasp, either.
Seems like on Windows 0 bits is handled as "Let Windows/Driver decide",
so handle that case by setting a reasonable value of 16.
Thanks to @SLAwww's explanations it's clear now :)
.. still, didn't look at unused variable warnings from *.es because
so many are generated.
and replace the corresponding conditions with more readable (but
equivalent) ones.
@DanielGibson
Copy link
Author

now it's mergeable again.

@icculus icculus merged commit be4c22f into icculus:master May 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants