Re: [Kaldi-developers] Windows patches to src/ (mostly)

[Kirill K. <kir...@sm...>]

I am not exactly "moving" main() into namespace kaldi: there is another, global main that calls kaldi::main(). The kaldi::main() can be named anything, and also I do not think the standard prohibits defining functions called main in other namespaces.

There are at least two ways to deal with the bug we discussed a few days ago:
1. Move everything into namespace kaldi. In this case, the namespace-local definitions (e. g. kaldi::int64) always win. Indeed, I am adding a trivial main() that calls kaldi::main() in each program.
2. Declare all used types explicitly. This is what I did in the couple files there, see the long banners of the using statements. Simply "using namespace kaldi" does not cut it.

Which of the two ways to select is a question I do not have much opinion on.

The error I am getting is about functions taking kaldi and openfst's declared types int32 and int64 by pointer or by reference being different and incompatible. I do not remember the exact message, but I can easily roll back the change and see, if it would make more sense.

 -kkm

> -----Original Message-----
> From: Daniel Povey [mailto:dp...@gm...]
> Sent: 2015-05-18 1944
> To: Kirill Katsnelson
> Cc: kal...@li...
> Subject: Re: [Kaldi-developers] Windows patches to src/ (mostly)
> 
> Thanks a lot for these.
> Yenda will deal with the checking-in issues.
> Generally speaking I like your patches, but I'll just reiterate here
> some comments I made on github:
> I'm not happy with the way you worked around the MSVC compiler bug by
> moving main() into namespace kaldi.  I believe this is not allowed,
> e.g. see here http://stackoverflow.com/questions/3956678/main-in-
> namespace,
> and even if it were allowed it seems very wrong to me; and I doubt it
> would compile on all platforms.  I'm OK to deal with MSVC's brokenness
> by adding various #ifdefs, but actually breaking Kaldi code is where I
> draw the line.  I'm sure there must be less heavyweight ways to deal
> with this problem?  I don't know what error message it was that you
> got, that you are trying to get rid of?
> 
> 
> Dan
> 
> On Mon, May 18, 2015 at 8:54 PM, Kirill Katsnelson
> <kir...@sm...> wrote:
> > I have collected all my source changes into 4 separate commits in my
> git fork. Please review the changes at the following links:
> >
> > The first change is trivial, but is necessary for the whole rig to
> work properly. EOL conventions are hard to fix later and cause the most
> confusion in version control.
> > https://github.com/kaldi-asr/kaldi/commit/bba053
> >
> > Compatibility changes for Windows cl (VS2013) and icl (Intel XE 2015
> > Update 3) https://github.com/kaldi-asr/kaldi/commit/1cc30d
> >
> > This is a change to fix the apparent MSVC bug with namespace
> > resolution in template expansion
> > https://github.com/kaldi-asr/kaldi/commit/f0f9e7
> >
> > And the last deals with proper I/O binarization under Windows, and
> Cygwin path conversions.
> > The change depends on the 2nd from top above.
> > https://github.com/kaldi-asr/kaldi/commit/5d2cc3
> >
> > There are also changes to openfst files and headers, but I do not
> know how to deal with them:
> >  - Check in the patches to be applied on top of existing patches?
> >  - Provide pre-build binaries?
> >  - Check in complete sources as part of kaldi repo?
> >
> > And there are build scripts, coming soon.
> >
> > Thanks,
> >
> >  -kkm
> >
> > ---------------------------------------------------------------------
> -
> > -------- One dashboard for servers and applications across
> > Physical-Virtual-Cloud Widest out-of-the-box monitoring support with
> > 50+ applications Performance metrics, stats and reports that give you
> > Actionable Insights Deep dive visibility with transaction tracing
> > using APM Insight.
> > http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
> > _______________________________________________
> > Kaldi-developers mailing list
> > Kal...@li...
> > https://lists.sourceforge.net/lists/listinfo/kaldi-developers