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

Enable RTTI and clean up the code by exploiting the availability of dynamic_cast. #409

Merged
merged 6 commits into from Jan 23, 2014

Conversation

lordhoto
Copy link
Contributor

This pull request enables RTTI (Run-Time Type Information) again and also exploits its availability in one case to clean up our code. For this, I made SdlGraphicsManager (virtually) inherit from GraphicsManager to reflect the actual conceptional inheritance tree. Additionaly, I moved activateManager/deactivateManager to SdlGraphicsManager since it's only used for subclasses of it. This would not have been possible without RTTI, because we can only do downcasts from GraphicsManager to SdlGraphicsManager, which are needed in the SDL code, in this case with dynamic_cast. The changes in SDL are not the only possible use where RTTI would be beneficial for code cleanness. For example, clone2727 wanted to use it in the WAV audio stream code (or some client) AFAIK.

In the past we disabled RTTI because of binary size [1](and incorrectly assumed per object dynamic size increase [2]). I made some quick check of commit 092d36f with gcc 4.8.2 and -O2 on Linux/amd64 to review the actual code size impact. The results for the stripped binaries are the following:
-fno-rtti, -O2: 25214544 bytes
with rtti, -O2: 25241872 bytes
As you can see the difference is "only" 27328 bytes which is roughly only 0.1% of the actual binary size! Thus, I think we can live with the additional static overhead and thereby gain a useful working dynamic_cast.

Any thoughts on this would be appreciated.

@clone2727
Copy link
Contributor

I would absolutely love to have RTTI enabled. Besides cleaning up the WAV code to finally try to cast to a SeekableAudioStream from there, there's a bunch of ADPCM and queuing audio stream stuff I'd like to clean up too. Specifically, it would make my life so much easier for both implementing seeking in Mohawk ADPCM streams and for cleaning up the QueuingAudioStream code to help with ZVision's ADPCM for AVI videos.

@clone2727
Copy link
Contributor

I would also be able to clean up the AIFF code too with a bunch of changes I've had dependent on dynamic_cast (Lots of audio-related things!). I'm sure if I keep thinking about it, I'll come up with even more reasons to have RTTI enabled :)

@wjp
Copy link
Contributor

wjp commented Oct 24, 2013

I thought the reason for having that disabled was lack of support in ancient PocketPC toolchains.

@fuzzie
Copy link
Contributor

fuzzie commented Oct 24, 2013

While I agree RTTI would be nice, can we perhaps test this with the buildbot toolchains first if possible? The Android one, at least, needed some config changes to support RTTI last time I poked at it (although nowadays it should work fine, so it's not a problem).

@fuzzie
Copy link
Contributor

fuzzie commented Oct 24, 2013

If we only have NDK r5b on the buildbot we might need to upgrade it to at least r8 to avoid things like http://code.google.com/p/android/issues/detail?id=28721 (which might give an idea of the reasons why I'd like the toolchains tested, sigh)

@lordhoto
Copy link
Contributor Author

I thought the reason for having that disabled was lack of support in ancient PocketPC toolchains.

0bad4c7 disabled RTTI and it seems not to talk about PocketPC toolchains. But of course, maybe there's some relation to some toolchains having problems?

@wjp
Copy link
Contributor

wjp commented Oct 24, 2013

That commit reads like: "We're not using it anyway, so let's disable it entirely to save space".

@lordhoto
Copy link
Contributor Author

That commit reads like: "We're not using it anyway, so let's disable it entirely to save space".

Yes. Our wiki on the other hand seems to say "we need the space thus we disable it" ;-).

@lordhoto
Copy link
Contributor Author

While I agree RTTI would be nice, can we perhaps test this with the buildbot toolchains first if possible?

That's a good idea but I suppose we would need to run test programs on actual hardware for this too?

@clone2727
Copy link
Contributor

http://sourceforge.net/mailarchive/message.php?msg_id=6800458 seems to be the only mention on -devel that I've seen, and Max is non-specific about what targets don't support it, however.

@wjp
Copy link
Contributor

wjp commented Oct 24, 2013

Chrilith has mentioned on IRC that PalmOS didn't support RTTI. ( http://logs.scummvm.org/log.php?log=scummvm.log.26Feb2008&format=html )

@criezy
Copy link
Member

criezy commented Oct 24, 2013

Compilers have evolved since 2004 or 2008 and legitimate reasons not to use it back then may not be valid anymore. Also the list of platforms we support has changed. Unless I am mistaken the PalmOS port is unmaintained and has seen no release since 0.11.1. Anyway I am guessing the persons who might know best whether there might be an issue with it or not are our porters for exotic platforms so I would suggest dropping an email in -devel (I am not sure they all monitor github pull requests).

@lordhoto
Copy link
Contributor Author

Unless I am mistaken the PalmOS port is unmaintained and has seen no release since 0.11.1.

In fact it's not in our source tree anymore.

@lordhoto lordhoto closed this Oct 25, 2013
@lordhoto lordhoto reopened this Oct 25, 2013
@lordhoto
Copy link
Contributor Author

oops sorry for the noise hit the wrong button :-P

@lordhoto
Copy link
Contributor Author

I made some compilation experiments on buildbot and it seems that the N64 and PS2 toolchains do not support RTTI/dynamic_cast (linking errors about missing __dynamic_cast).

@lordhoto
Copy link
Contributor Author

Just a small update: With the help of Hkz and sunmax I was able to actually compile and link a small test program which uses dynamic_cast/RTTI just fine for N64 and PS2. So, that shouldn't be an problem at least.

@lordhoto
Copy link
Contributor Author

Finally got some replies by both Hkz and sunmax (thanks!) that RTTI is working on N64 and PS2. So, I think we should be able to merge this now? (We should still take care of updating our Android toolchain on buildbot though).

@digitall
Copy link
Member

Have upgraded Android NDK to r9c (latest) on buildbot. Testing now. If build is successful, then feel free to merge.

@digitall
Copy link
Member

Yep. Build successful, so OK to merge.

@lordhoto
Copy link
Contributor Author

Have upgraded Android NDK to r9c (latest) on buildbot. Testing now. If build is successful, then feel free to merge.
Yep. Build successful, so OK to merge.

Great thanks!

If there's no other objections, I will merge this one.

@bluegr
Copy link
Member

bluegr commented Jan 22, 2014

Looks good to me, it would be nice to have RTTI enabled and allow for more flexibility.

Two questions:

  • why didn't the N64/PS2 backends not work initially? Is there any configuration needed to get RTTI working there? Perhaps this should be mentioned somewhere, in case people want to create their own builds?
  • are there any changes/recompilation needed to external libraries (either the ones that are statically linked, or dynamically linked)? Probably not, but it doesn't hurt to ask anyway :)

@lordhoto
Copy link
Contributor Author

why didn't the N64/PS2 backends not work initially? Is there any configuration needed to get RTTI working there? Perhaps this should be mentioned somewhere, in case people want to create their own builds?

I tried to compile some stand alone test program. The toolchains are very sensitive to parameters passed when linking, I missed some bits when testing on my own. We will of course make sure that it works out of the box.

are there any changes/recompilation needed to external libraries (either the ones that are statically linked, or dynamically linked)? Probably not, but it doesn't hurt to ask anyway :)

There shouldn't be any issues, after all all our libraries we use have C interfaces too anyway...

Thanks to fuzzie for these changes.
lordhoto added a commit that referenced this pull request Jan 23, 2014
Enable RTTI and clean up the code by exploiting the availability of dynamic_cast.
@lordhoto lordhoto merged commit 2fe303c into scummvm:master Jan 23, 2014
@lordhoto lordhoto deleted the rtti branch February 11, 2014 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants