On Fri, Apr 17, 2009 at 9:17 AM, Alexei Sheplyakov <varg@metalica.kh.ua> wrote:
Thanks for a patch. Supporting more platforms/compilers is certainly a good thing as long as it doesn't break the code and doesn't introduce subtle changes. Unfortunately your patch *does* introduce subtle changes (and breaks compilation with any non-m$ compiler).
Note that it is not intended to be applied as-is to ginac source tree.
Anyway, it would be nice if you avoid introducing the breakage and semantic changes, or at least clearly mark them.
You know, it took me some time to track down incorrect usage of STL iterators in ginac sources. I tried to fix the ones I could find (because test suite crashed), but I'm sure there are more of them. Yes, it's possible that I introduced other problems, but I have no knowledge of ginac and what it's doing internally, so those fixes are just blind guesses. The patch was more intended to identify problems and propose a naive solution (which at least you know works with MSVC). If you don't want to use it, then don't.
diff -ur ginac-1.5.1/ginac/ncmul.cpp ginac-1.5.1-new/ginac/ncmul.cpp --- ginac-1.5.1/ginac/ncmul.cpp 2009-02-17 13:39:22 +0000 +++ ginac-1.5.1-new/ginac/ncmul.cpp 2009-04-15 23:14:00 +0100 @@ -340,7 +340,7 @@
// determine return types unsignedvector rettypes; - rettypes.reserve(assocseq.size()); + rettypes.resize(assocseq.size());
I'm afraid this change breaks canonicalization of non-commutative products.
I'm afraid that setting vector's capacity doesn't allow you to reference elements outside of the actual vector's size.
Some comments about the patch: - the __alignof__ vs. __alignof could be turned into a configure test
The patch as is breaks compilation with any non-m$ compiler. That's certainly not welcome.
I know it breaks non-VC compiler. That's why I mention the use of a configure test. But I leave that up to you.
- VC++ does not support __func__ macro, so I changed it into __FILE__
Stripping debugging info is not welcome either. Please #ifdef _MSVC (or whatever it is) these changes.
- in exprseq.cpp, I had to add a dummy function to make VC++ instantiate expreseq::info(); don't know why...
Please #ifdef _MSVC that function. Also it would be nice to have a comment in the source explaining why it's necessary (otherwise it's so tempting to delete that pointless code). Other changes, like
diff -ur ginac-1.5.1/ginac/parser/parser.cpp ginac-1.5.1-new/ginac/parser/parser.cpp --- ginac-1.5.1/ginac/parser/parser.cpp 2009-02-17 13:39:22 +0000 +++ ginac-1.5.1-new/ginac/parser/parser.cpp 2009-04-15 20:59:47 +0100 @@ -82,7 +82,7 @@ }
extern numeric* _num_1_p; -extern ex _ex0; +extern const ex _ex0;
need such an #ifdef and a comment too.
Why? The exported symbol uses the "const" modifier. Declaring it non-const like this implicitely assumes that the C++ mangled symbols (const and non-const) will be the same. That's not the case in VC++.
P.S. (How) Did you manage to compile CLN with msvc?
Magic. VC++ per-se is quite (not fully) standard compliant. Most of the problems are in the autotool based build framework. With a bit of coding, you can make VC++ look like GCC. Michael.