Re: [lmi] building with Visual C++

[Greg Chicares]

On 2008-01-18 21:40Z, Vaclav Slavik wrote:
> 
> I was trying to reproduce the mysterious crash in product editor 
> reported some time ago (with no luck, unfortunately) and because 

I might have made some changes to turn that segfault into an
orderly error message, by detecting the circumstances that caused
the segfault and throwing a C++ exception. Here's what happens
today when I load 'sample.db4':

Error
Trying to index database with key 0: e_number_of_axes is 7, and 
axis_lengths.size() is 196609, but those quantities must be equal.
[file /lmi/src/lmi/ihs_dbvalue.cpp, line 301]

To reproduce that, load the program and then immediately load
that file; if you do anything else first, you probably won't see
the error message.

I believe that this instance of class TDBValue comes into being
in the product editor without being constructed--something like
  class C { void foo(){} };
  C* c; // Uninitialized.
  c->foo();
IIRC, a while ago I experimentally (outside of cvs) added sanity
checks to all the ctors, but that didn't trap the segfault.

> nothing showed up under Valgrind on Linux and Valgrind isn't 
> available for Windows, I tried to compile LMI with Visual C++ so that 
> we can run the app under Purify or other checkers (none that I know 
> about works with Mingw).
> 
> Here's a patch I had to apply to make the code compile with Visual C++ 
> 2005 (including necessary makefiles):
> http://public.vslavik.fastmail.fm/lmi.vc.patch.gz
> 
> The patch is just ugly quick hack to make it compile, it disables some 
> things when fixing them would be too hard (in particular, there's a 
> hack in place to make xml::node's private methods public and it 
> doesn't fool VC). Some of the changes in this patch would be trivial 
> to fix (e.g. code using e_mode on right side of operator / doesn't 
> work, but that could be fixed by adding global operator/),

On 20080121T0132Z and 20080121T1400Z I applied the following
trivial changes.

'ce_product_name.cpp': Applied 20080121T0132Z. A search for other
instances of the same problem led me to change several other
files at the same time.

'ihs_basicval.cpp': Applied 20080121T1400Z. I changed two other
instances of division by 'e_mode' in the same file, as well:

+        spread = MinPremIntSpread_[Year] * 1.0 / Mode.value();
-        spread = MinPremIntSpread_[Year] * 1.0 / Mode;
+    return (1.0 - std::pow(u, 12.0 / Mode.value())) / (1.0 - u);
-    return (1.0 - std::pow(u, 12.0 / Mode)) / (1.0 - u);

I wonder why those two cases, and other similar cases in other
files, weren't problems for msvc. I didn't change the other files
only because I plan to get rid of type 'e_mode' this year.

> but some 
> would mean big changes -- for example, the compiler gets terribly 
> confused about mc_enum<> template's implementation in mc_enum.tpp, 
> but compiles fine if the code is moved to be inline in class' body.

As for the big changes in class template mc_enum, I'm wondering
why msvc dislikes the original code in HEAD. I used to restrict
myself to code that worked with gcc-2.x and borland, which meant
living in a straitjacket. Today, with gcc-3.4.x and comeau, I can
pretty much use the whole standard C++ language. I've left some
borland support in place because it'll still compile many of the
unit tests, but I don't try to maintain borland compatibility,
which became too confining. This is one of the things borland
just can't handle at all, but I was hoping it would work with
any compiler that can handle boost and Loki.

For template class mc_enum, I chose explicit instantiation for
good reasons. I know I measured an improvement in compile or link
times, though I failed to document that and can't say whether the
technique is better with more recent compilers. I think there was
some advantage in compile-time error detection, too. A separate
benefit is writing member-function bodies out of line, which I
think makes the code easier to read.

I'm not sure whether it's the explicit instantiation, or the out-
of-line function bodies, that msvc complains about--or maybe it's
both in combination. If you can point out some way in which this
doesn't conform to the language standard, that's enough reason to
change the code. Otherwise, if it's just a conformance failure of
msvc, then I'd still consider changing it, with some reluctance
though; but first we'd need to weigh the costs and benefits of
getting the whole system to build with that compiler....

> Do you think it's worth having LMI buildable using VC++, should we go 
> on and maintain this build?

As for msvc:

 - It would be nice to support one more compiler, mostly because
   that lets us find more actual errors earlier, when they're
   easy to fix. This newsgroup thread:

http://groups.google.com/group/borland.public.cppbuilder.language.cpp/browse_thread/thread/923513e67ec1d91b/478b1c80711dadb5

   offers a motivating example. I had written some code that both
   gcc and comeau accepted, but the unit test failed with borland
   tools. My original code was wrong: IIRC, the standard permits
   the other compilers to do what I expected without emitting any
   diagnostic, but doesn't guarantee that it'll work.

 - It's also an advantage if an additional compiler has (or works
   with) useful tools for error detection beyond usual compiler
   diagnostics, as in the case you mention.

 - OTOH, it's not relevant that msvc is popular in some segment
   of the non-free (software) world: lmi needs to support a
   monopoly compiler "like a fish needs a bicycle".

> (Obviously not with this patch, there 
> would have to be proper fixes instead of the workarounds I did.)

Here are my comments and questions on the rest of the patch.

'xslt_lmi.cpp': This should be completely replaced; we should use
the 'xsltwrapp' library instead.

'multidimgrid_safe.?pp': The patches for these files won't apply
to HEAD; I think they patch some modified versions not in HEAD.

'wx_new.hpp': I assume this is just a quick and dirty way of
getting msvc working, and shouldn't be applied to HEAD.

'datum_base.hpp': I don't understand these changes:

+    // FIXME: VC++ complains about lack of this. Looks suspicious, the comment
+    // above about implicit versions of these methods may be wrong...
+    bool operator==(const datum_base& d) const;

The comment referred to is
  // Implicitly-declared special member functions do the right thing.
C++2003 12, paragraph 1, defines which are "special", and the
equality-comparison operator isn't one of them. But how can it
help to declare this member function and not implement it? Is
msvc using something like 'guiding-decls' from pre-standard C++?

-    virtual std::istream& read (std::istream&)       = 0;
-    virtual std::ostream& write(std::ostream&) const = 0;
+    // FIXME: this looks like VC++ is trying to make *instances* of datum_base
+    //        from inside value_cast<> code, which would be incorrect, 
datum_base
+    //        copy ctor is not OK!
+    virtual std::istream& read (std::istream& s)       {return s;}//= 0;
+    virtual std::ostream& write(std::ostream& s) const {return s;}//= 0;

If there's a real standard-C++ problem here, I'd like to fix it.
But in what way is the implicitly-defined copy ctor not okay?
Are you sure that's the actual (msvc) problem?