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

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

Kirill,  did you tried to compile against the patched openfstwin? I think I
addressed the type lookup issues.
Y.
On May 19, 2015 10:31 PM, "Kirill Katsnelson" <
kir...@sm...> wrote:

> I am going back to reproduction the compile error in MSVC. I am using
> bin/phones-to-prons as a simple example and have reduced the module to the
> following exact lines:
>
> #include "fst/fstlib.h"
> #include "fstext/fstext-lib.h"
> int main(int argc, char *argv[]) {
>   using namespace kaldi;
>   fst::VectorFst<fst::StdArc> phn2word;
>   fst::FstPrinter<fst::StdArc> fstprinter(phn2word, NULL, NULL, NULL,
> false, true);  // line 6
> }
>
> The error message follows. Only MSVC reports it, the Intel compiler has no
> problem here:
>
> C:\projects\kaldi\kaldi\extlibs\openfstwin-1.3.1\include\fst/script/print-impl.h(77):
> error C2872: 'int64' : ambiguous symbol
> [C:\projects\kaldi\kaldi\msbuild\bin\kaldi-bin.kwproj]
>           could be
> 'C:\projects\kaldi\kaldi\extlibs\openfstwin-1.3.1\include\fst/types.h(31) :
> int64_t int64'
>           or       'C:\projects\kaldi\kaldi\src\base/kaldi-types.h(48) :
> kaldi::int64'
>           ..\../src/bin/phones-to-prons.cc(6) : see reference to class
> template instantiation 'fst::FstPrinter<fst::StdArc>' being compiled
>
> The line print-impl.h(77), containing the reportedly ambiguous symbol, is
> essentially this:
>
> namespace fst {
> template <class A> class FstPrinter {
>   void PrintId(int64 id, const SymbolTable *syms, const char *name) const
> {  // Line 77
>   }
> };
> }
>
> This is indeed a compiler bug. The int64 does not even come from a
> template parameter, and is unambiguously ::int64 in this scope, already
> defined in an fst header.
>
> Now, you would think that the template is expanded in the lexical scope of
> invocation, and thus preventing kaldi::int64 from being known as just int64
> at this point would be the fix. Yes it is a fix indeed, but also is putting
> everything into the kaldi namespace. This compiles without an error (then
> the linker complains about the missing _main, but this is not a problem at
> this point):
>
> #include "fst/fstlib.h"
> #include "fstext/fstext-lib.h"
> namespace kaldi {
> int main(int argc, char *argv[]) {
>   fst::VectorFst<fst::StdArc> phn2word;
>   fst::FstPrinter<fst::StdArc> fstprinter(phn2word, NULL, NULL, NULL,
> false, true);
> }
> }
>
> Why this works I do not know. Perhaps, kaldi::int64 is used in the
> template expansion. But it is also a fix (or, better put, a workaround).
>
> I agree that the fst library goes way too far when defining global types
> int64 and friends (fst/types.h). But I do not know if it is possible to
> change at this point. If it is redefined as "fst::int64", kaldi is going to
> have trouble in modules having both "using namespace kaldi" and "using
> namespace fst", I think.
>
> An amazingly ugly fix would be to define int64 as an alias to kaldi::int64
> etc., and then #define FST_LIB_TYPES_H__ before including fst headers, thus
> preventing fst's definitions. I certainly would not like to depend on an
> internal detail of the fst library though.
>
>  -kkm
>
> > -----Original Message-----
> > From: Daniel Povey [mailto:dp...@gm...]
> > Sent: 2015-05-18 2011
> > To: Kirill Katsnelson
> > Cc: kal...@li...
> > Subject: Re: [Kaldi-developers] Windows patches to src/ (mostly)
> >
> > Hm.
> > If the only issue is the int32and int64 definitions, I think a cleaner
> > fix would be to apply a patch to OpenFst to make it use the same
> > definitions that Kaldi uses (which is the "proper" way to do it).
> > Dan
> >
> >
> > On Mon, May 18, 2015 at 11:05 PM, Kirill Katsnelson
> > <kir...@sm...> wrote:
> > > 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
> > >> > 50+ 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
>
> ------------------------------------------------------------------------------
> 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
>