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

[Daniel P. <dp...@gm...>]

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