kokoye2007 / waitzar

Automatically exported from code.google.com/p/waitzar
Other
0 stars 1 forks source link

Upgrade WZ's code base for Eclipse CDT #137

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Currently, WZ _can_ be compiled with VS2010, but you have to set the "Platform 
Toolset" to "v90". 

Unfortunately, this requires Visual Studio 2008 to be installed, since 
(apparently) VS2010 uses a new C++ build system. 

Add that to the various minor platform-breakers introduced in C++0x, and you 
get the need for a massive overhaul of the code base.

Two main issues to address:
  1) const_casts are needed for ANY set::iterator, since these are now const by default. This does NOT invalidate the reason why they are now const (to preserve sort order), so it's just a matter of find-and-replace-and-deal-with-sets-of-values-instead-of-pointers.
  2) Some size_t data are now "streamsize", or something like that. In most cases, the new values are never less than zero, so they can safely be cast to (the unsigned) size_t. Note that this might be a VS thing instead of a C++0x thing, and it only generates warnings. 

There are probably more, but these are the first 2 I saw.

Probably best to do this for WZ 1.9+? It doesn't really affect much, and VS2008 
Express is still available for download from Microsoft, presumably because of 
the compiler dependency mentioned above.

Original issue reported on code.google.com by seth.h...@gmail.com on 3 Sep 2010 at 5:04

GoogleCodeExporter commented 9 years ago
Here's another option:
  1) Use Eclipse CDT
  2) Use mingw+gdb on windows

I used to be strongly against this. However, there are some positives:
  1) gcc (mingw) means that we will always have the latest version of C++ (note that VS _still_ doesn't fully support some C99 features   ...and as far as c++0x goes, gcc is WAY ahead, with things that WZ really needs).
  2) gdb should work in Eclipse. (See: http://www.qtforum.org/article/24952/a-guide-to-install-mingw-qt4-and-eclipse-integration-on-windows-xp.html)

The only things I need for VS are, in order of necessity:
   1) Debugging
   2) Windows platform integration (e.g., resources, manifests)
   3) Fast & efficient optimization
   4) Auto-complete integration (various WM_* flags, etc.)

My thoughts:
   1) Supposedly Eclipse can do this. I would have to try it myself, though, since I'm picky when it comes to debugging.
  2) I feel that this can be done with a small amount of work. This is more of a fun challenge than a real problem.
  3) I know that mingw is nearly on par with VS; sometimes it's faster. We could also profile WZ in mingw if we really wanted to, although I doubt it'll be much slower. Also, UPX can handle file size.
  4) We'll get some of this from Eclipse, but at this point, I'm willing to sacrifice 4 in exchange for initializer lists ALONE.

The only other problem is that this approach might alienate new developers. 
However, I think anyone who's smart enough to compile WaitZar on VS should be 
able to handle Eclipse (they likely know Java) and would probably need no more 
than a simple "How to set up the CDT" section in the WaitZar developer's guide. 

After the next release of WZ, I'll try porting the build environment, and see 
if this is a workable solution. 

Original comment by seth.h...@gmail.com on 7 Sep 2010 at 6:00

GoogleCodeExporter commented 9 years ago
Some notes on early porting attempts to Eclipse:
  * We need to add "." and "./Contrib" to the path, as we do in VS.
  * Boost sometimes fails to find files included with angle brackets. Def. need to fix this.
  * Need to use runtime_error instead of exception.
  * Need to catch by reference, or else we lose the "what" of runtime_error
  * Need to find a safe, non-leaking replacement for alloca()

So far, not so bad. Upgrading after this to C++0x, of course, will be another 
bag of fun.

Original comment by seth.h...@gmail.com on 21 Sep 2010 at 10:16

GoogleCodeExporter commented 9 years ago
Regarding alloca, we can't just switch it to malloc (since alloc'd memory frees 
itself). 
So, we can do this:

#ifdef __MINGW32__
void *alloca(size_t);
#endif

...assuming that the code will still link.

Or maybe we could wrap a call to malloc in some kind of auto-pointer? 

Original comment by seth.h...@gmail.com on 6 Oct 2010 at 9:55

GoogleCodeExporter commented 9 years ago
>Or maybe we could wrap a call to malloc in some kind of auto-pointer? 

Unless it's an array. (Whoops...)

Original comment by seth.h...@gmail.com on 6 Oct 2010 at 9:57

GoogleCodeExporter commented 9 years ago
Ok, got it. Apparently, VS includes malloc() by default. So just change 
rinjdael.cpp (line 72 or so) from:

#ifdef __sun
  #include <alloca.h>
#endif

...to:

#ifdef __sun 
  #include <alloca.h> 
#else 
  #if defined(__APPLE__) 
    #include <stdlib.h> 
  #else 
    #include <malloc.h> 
  #endif 
#endif 

So this is not actually a mingw bug.

By the way, interesting quote from the Crypto++ dev:

>Can we replace alloca with malloc/free?
No, please don't do that. Alloca is being used to find some stack space that 
isn't cache-aliased with static lookup tables, in order to defend against 
timing attacks. It's easy to just apply Zooko's fix. I'll also get this fixed 
in the next release.

Will test later.

Original comment by seth.h...@gmail.com on 6 Oct 2010 at 10:02

GoogleCodeExporter commented 9 years ago
The binutils equivalent of rc.exe is "windres". For example:
http://blog.stranadurakov.com/tag/windres/

We'll have to use this for all our embedded resources.

Original comment by seth.h...@gmail.com on 7 Oct 2010 at 6:19

GoogleCodeExporter commented 9 years ago
More concise example, although they put the resource file in its own directory:

http://stackoverflow.com/questions/1411040/how-to-add-an-icon-to-an-application-
built-with-eclipse-galileo-c-and-mingw

Original comment by seth.h...@gmail.com on 7 Oct 2010 at 6:25

GoogleCodeExporter commented 9 years ago
Change of title. 

Some initial warnings include things like "#pragma comment" in CrypLib. This is 
used (among other things) to auto-include a library. 

I'm happy to fix some of these myself where they represent laziness (e.g., 
size_t and int comparisons). Others may remain, so long as they are not errors. 

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 6:42

GoogleCodeExporter commented 9 years ago
#include <stdexcept> is needed to recognize exceptions. 

I'd like to include this project-wide, but I can't find a way of doing that in 
Eclipse CDT. Adding a single line to PulpCoreFont.h seems to clear up the 
majority of the compiling issues with stdexcept.

In addition, the following key values were not defined:
VK_OEM_PLUS 0xBB
VK_OEM_COMMA    0xBC
VK_OEM_MINUS    0xBD
VK_OEM_PERIOD   0xBE

I defined them in the project defaults (-D) and that seemed to work. The 
project's big, though; won't know for sure until it compiles completely.

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 7:07

GoogleCodeExporter commented 9 years ago
Had to disable "urlmon.h" include (used for downloading files) as it is 
currently not in MinGW. 

I'm perfectly happy replacing this with a more cross-platform friendly lib. 

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 7:11

GoogleCodeExporter commented 9 years ago
The "IsAdmin" function in "MiscUtils" uses "__leave", which is a non-standard 
kind of break-from-try imperative. 

But apparently "__try" and "__finally" are NOT used for exceptions, but for 
something else? Seems like a Win32-specific exception device. Should be easy to 
re-write; I haven't touched IsAdmin for unrelated reasons. 

Just commented it out to return true for now. A reported better solution is:

__try
{
   // code 1

   if ( ... )
      __leave;

   // code 2

   if ( ... )
      __leave;

   // code 3
}
__finally(true)
{
   // code 4
}

is equal to:

   // code 1

   if ( ... )
      goto clean;

   // code 2

   if ( ... )
      goto clean;

   // code 3

clean:
   // code 4

...from:
http://www.experts-exchange.com/Programming/Languages/C/Q_21793519.html

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 7:16

GoogleCodeExporter commented 9 years ago
Various minor include issues (like "Settings/ConfigManager.h" in WZFactory.h) 
were fixed. Apparently VS is generous how it manages contexts? 

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 7:22

GoogleCodeExporter commented 9 years ago
In a few cases, Visual Studio (correctly) assumed that I was using a templated 
type. This required me to replace, e.g.:

std::set<T>::iterator it=container.begin();

with:

typename std::set<T>::iterator it=container.begin();

in MinGW-targeted code. No big deal; I should have been doing this anyway.

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 7:26

GoogleCodeExporter commented 9 years ago
Note that, in reference to comment 12, it is possible the ConfigManger.h was 
compiling incorrectly, and thus could not be found for include. (I imagine this 
is likely, but, again, can't test it until the entire project compiles 
successfully.)

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 7:28

GoogleCodeExporter commented 9 years ago
MinGW seems to require WINVER to be set instead of _WIN32_WINNT. I've added the 
following #define to MainFile.cpp:
#define WINVER _WIN32_WINNT 

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 7:40

GoogleCodeExporter commented 9 years ago
SHGFP_TYPE_CURRENT and CSIDL_MYDOCUMENTS were not declared in shlobj.h.

Added ifdef'd definitions in MainFile. (Should probably migrate the OEM_COMMA 
et. al. definitions to MainFile later, with ifdef guards).

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 7:51

GoogleCodeExporter commented 9 years ago
Actually, our defines fix for AlphaBlend didn't work. Needed to put this:
  #define _WIN32_WINNT 0x0500
  #define WINVER _WIN32_WINNT
  #define _WIN32_IE 0x0501

...in front of our project-level defines. (Something was weird about including 
windows.h... did it undefine things somehow?)

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 7:58

GoogleCodeExporter commented 9 years ago
ConfigManager uses map::find, and it apparently returns a const_iterator. 
However, this _should_ be allowed, since map.find() can return a regular 
iterator. 

I'm happy to replace iterator with const_iterator, but I dislike not knowing 
the full cause of the problem. 

My guess is that _one_ of the various iterators is tainting the rest with a 
"const" specification. (We have to completely redo set iterators for C++0x 
compatibility anyway, so I guess I'm not too upset about this error.)

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 8:06

GoogleCodeExporter commented 9 years ago
Added NOMINMAX to the set of project-wide defines. 

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 8:07

GoogleCodeExporter commented 9 years ago
Extracted our own "min" and "max" functions into their own file, so that our 
own local includes could take advantage of them.

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 8:11

GoogleCodeExporter commented 9 years ago
Several header file definitions (notably in ConfigManager) neglect the std:: 
prefix before map/wstring. I'm not sure how this even worked under VS.

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 8:17

GoogleCodeExporter commented 9 years ago
ODS_HOTLIGHT and NIN_BALLOONUSERCLICK also need to be defined. 

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 8:19

GoogleCodeExporter commented 9 years ago
WZFactory<ModelType>::systemDefinedWords 

...is used in SentenceList, but this would create a cyclical dependency.

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 8:38

GoogleCodeExporter commented 9 years ago
In a few cases, the LPSTR returned by MAKEINTRESOURCE was flagged as being 
misused as a WORD. Since this is the "Windows" way of dealing with 
MAKEINTRESOURCE's result, you can guess what compiler would be less strict 
about this...

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 8:44

GoogleCodeExporter commented 9 years ago
Ok, I think I tracked down all that "discards qualifiers" nonsense. Normally, 
if you mix const and non-const iterators you're bound to run into this. I'd 
avoided that by making all my iterators non-const... or so I thought. 

But here:
  std::set<Encoding>::iterator defEnc = lg->encodings.find(lg->defaultOutputEncoding);
  lg->defaultOutputEncoding = *defEnc;

...we have a set returning an iterator, whose value is then used as the new 
default output encoding. Note that lg is defined as:

   std::set<Language>::iterator lg=options.languages.begin();

...I'm note 100% sure which iterator is causing the issue (but I bet it's 
lg->defaultOutputEncoding). 

I could const-cast around this, perhaps. Or something else. Not sure what to do 
at the moment (for the test-compile). 

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 8:51

GoogleCodeExporter commented 9 years ago
Hyperlinks.cpp has the following error:

hCursor = LoadCursor(NULL, MAKEINTRESOURCE(reinterpret_cast<WORD>(IDC_ARROW)));

The problem here is that IDC_ARROW is _already_ "MAKE'd" with MAKEINTRESOURCE 
(it's a string) and casting it back to a WORD just to get the _same_ string is 
a bad idea (pointers might be bigger than a WORD). 

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 9:21

GoogleCodeExporter commented 9 years ago
FYI, updating of const casts can be done like so:

   lg->defaultOutputEncoding = *defEnc; 

...becomes:

   const_cast<Language&>(*lg).defaultOutputEncoding = *defEnc;

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 9:26

GoogleCodeExporter commented 9 years ago
Need to add "GDI32" to the Linker. Other libraries too:

msimg32.lib
Kernel32.lib
shell32.lib
Imm32.lib
urlmon.lib  //Excluding this one for now.

Original comment by seth.h...@gmail.com on 12 Jan 2011 at 9:44

GoogleCodeExporter commented 9 years ago
Disabled Crypto++ for now. 

I think that we might consider putting Crypto++ into its own project. The .lib 
file is 2.5MB and the headers are 0.5, so it's not too bad. Plus, a lot of 
Crypto is causing warnings that we don't care about.

The only downside to this is that there's still tons of headers to include with 
the .lib, so we don't gain as much separation as we'd like. 

Possible alternative: OpenSSL?
http://www.openssl.org/docs/crypto/md5.html

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 1:38

GoogleCodeExporter commented 9 years ago
There's an error in Helios's error parser:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=318093

Need to fix this; instantiation is useful but not an error. (But was reported 
in June and has been closed; maybe this has been fixed?)

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 2:16

GoogleCodeExporter commented 9 years ago
Done! Wait Zar compiles completely on Eclipse in 143s (2.4 min.)

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 2:29

GoogleCodeExporter commented 9 years ago
Using:
   windres WaitZarRes.rc -O coff -o ../WaitZarRes.o

Requires the following files in a new "res" subdir:
   close_x.png
   default-config.json.txt
   easypatsint.txt
   help_ar_letters.font.png
   help_icon.bmp
   help_zg_main.font.png
   help_zg_sub.font.png
   keybd_corner.png
   loading.ico
   Myanmar.model
   pdkzg.ttf
   pg_down_color.png
   waitzar.font.png
   WaitZarRes.rc
   waitzar_small.font.png
   wz.ico
   wz_eng.ico
   wz_mm.ico

...and a link back to "../resource.h". It puts the resulting WaitZarRes.o file 
into the parent directory, so we can ignore "res" in the build dir. 

The task is performed very quickly, so we can do it every time. (Or, we might 
check if WaitZarRes has changed... or we might do it only on "clean". Anyway.)

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 2:46

GoogleCodeExporter commented 9 years ago
(actually it looks more like)

windres ../res/WaitZarRes.rc -O coff -o ../res/WaitZarRes.o

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 2:55

GoogleCodeExporter commented 9 years ago
Building in Release mode takes:

220s (3.6 min., not much worse than Debug)

EXE is 3.76MB. Ouch! (Also doesn't include Crypto++). 

upx'd EXE is 2.14 MB, so I'm thinking that MinGW is bundling lots of extra 
useless information. 

Running "Strip" on WaitZar.exe reduces the size to:
2.09 MB

upx-ing that brings us to:

From VCC to MinGW, we go from:
Size: 1.7 -> 2.1
UPX:  0.5 -> 0.4  (huh??)

Seems decent. I'll add "-s" to the linker's options.

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 3:34

GoogleCodeExporter commented 9 years ago
File size goes down to 1.7MB if we set "-Os" instead of "-O3". Note that Os 
sets the same flags as O2, which is supposedly pretty good. 

Running the slowest noticeable part of the program (Key Magic) results in.... 
well, a crash due to our MD5 code. But myWin2.2 seems fast enough. 

So, for now, we'll set -Os on the Release build.

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 3:49

GoogleCodeExporter commented 9 years ago
Note that the following linker flag:
-Wl,-subsystem,windows 

...is needed to prevent the MinGW program from using a console.

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 4:07

GoogleCodeExporter commented 9 years ago
Added 3 new bugs for our most serious migration issues. 

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 6:11

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
NOTE: For C++0x, there's a few more changes:
  1) Remove all references to swprintf. (Happy to! Also, done!)
  2) Fix some problems in spirit_json. For example, "function" should be declared as "boost::function". My guess is that this is just regular namespace collision. 

There might be more. I'm not hell-bent on enabling C++0x right away, but I'd 
like to leave the possibility open in the future. 

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 9:28

GoogleCodeExporter commented 9 years ago
....or it already works? Weird; I expected a harder migration.

Anyway, that was nice. 

Original comment by seth.h...@gmail.com on 13 Jan 2011 at 9:30

GoogleCodeExporter commented 9 years ago
Migration complete. 

Original comment by seth.h...@gmail.com on 14 Jan 2011 at 9:07