openfst
openfst copied to clipboard
kkm000
error C2995: 'void fst::Prune(fst::MutableFst<A> *,const fst::PruneOptions<Arc,ArcFilter> &)': function template has already been defined
When compiling fstscript I'll get a bunch of errors saying:
1>C:\Users\jtrmal\Documents\openfst\src\include\fst/prune.h(216): error C2064: term does not evaluate to a function taking 0 arguments
1>C:\Users\jtrmal\Documents\openfst\src\include\fst/prune.h(305): error C2064: term does not evaluate to a function taking 0 arguments
1>C:\Users\jtrmal\Documents\openfst\src\include\fst/prune.h(311): error C2995: 'void fst::Prune(const fst::Fst<Arc> &,fst::MutableFst<A> *,const fst::PruneOptions<Arc,ArcFilter> &)': function template has already been defined
I was playing with that and seems it's because of the enable_if magic. After changing
typename std::enable_if<
(Arc::Weight::Properties() & kPath) == kPath>::type * = nullptr
to
typename = std::enable_if<
(Arc::Weight::Properties() & kPath) == kPath>::type
(as http://en.cppreference.com/w/cpp/types/enable_if describes) I end up with the error
error C2995: 'void fst::Prune(const fst::Fst<Arc> &,fst::MutableFst<A> *,const fst::PruneOptions<Arc,ArcFilter> &)': function template has already been defined
which I think relates to "Notes" in here: http://en.cppreference.com/w/cpp/types/enable_if
but from there, I have no idea if there is an easy way how to resolve it
Let me try to repro. What is the cc file you are compiling? And VS 2015 or 2017?
arciterator-class.cc and I'm using MSVC 17 (Community Edition)
I have found discussion here: https://stackoverflow.com/questions/26753393/what-is-wrong-with-this-use-of-stdenable-if that seems to make sense y.
On Mon, Nov 6, 2017 at 3:54 PM, Kirill Katsnelson [email protected] wrote:
Let me try to repro. What is the cc file you are compiling? And VS 2015 or 2017?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/6#issuecomment-342283386, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisXyRgFxBpWdPJuvor9uOisDw6lehfks5sz3IigaJpZM4QT2jT .
Yep, I am reproducing it by simply compiling this file! Let's see if I may have any ideas too.
This is the weird part:
error C2039: 'type': is not a member of 'std::enable_if<false,_Ty>'
It's like BUT OF COURSE it's not, that's the whole point of this template... Very strange.
changing the functions like this:
template <class Arc, class ArcFilter>
typename std::enable_if<(Arc::Weight::Properties() & kPath) != kPath>::type
Prune(const Fst<Arc> &, MutableFst<Arc> *ofst,
const PruneOptions<Arc, ArcFilter> &) {
FSTERROR() << "Prune: Weight needs to have the path property: "
<< Arc::Weight::Type();
ofst->SetProperties(kError, kError);
}
worked partially -- while for some function the issue got resolved, I still have issues with
1>C:\Users\jtrmal\Documents\openfst\src\include\fst/prune.h(308): error
C2995: 'std::enable_if<,void>::type fst::Prune(const fst::Fst<Arc>
&,fst::MutableFst<A> *,const fst::PruneOptions<Arc,ArcFilter> &)': function
template has already been defined
1>C:\Users\jtrmal\Documents\openfst\src\include\fst/prune.h(215): note: see
declaration of 'fst::Prune'
which is fairly confusing as it seems there that both the condition and the negation of the condition are fullfiled and with WeightCompare which I don't know how to deal with, as it returns bool
1>C:\Users\jtrmal\Documents\openfst\src\include\fst/isomorphic.h(48): error
C2995: 'bool fst::internal::WeightCompare(Weight,Weight,float,bool *)':
function template has already been defined
1>C:\Users\jtrmal\Documents\openfst\src\include\fst/isomorphic.h(28): note:
see declaration of 'fst::internal::WeightCompare'
On Mon, Nov 6, 2017 at 4:12 PM, Kirill Katsnelson [email protected] wrote:
This is the weird part:
error C2039: 'type': is not a member of 'std::enable_if<false,_Ty>'
It's like BUT OF COURSE it's not, that's the whole point of this template... Very strange.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/6#issuecomment-342288168, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisXyNduNdLthhpUQ361B6_KLmi2z2nks5sz3YngaJpZM4QT2jT .
It looks like MSVC cannot digest that. Other compilers can.
But the deeper issue is why are they using enable_if; in this case it has no point.
I believe the best way to handle this is merge the two templates like
// Plus(weight, Weight::One()) == Weight::One()
template <class Arc, class ArcFilter>
void Prune(MutableFst<Arc> *fst, const PruneOptions<Arc, ArcFilter> &opts) {
using StateId = typename Arc::StateId;
using Weight = typename Arc::Weight;
using StateHeap = Heap<StateId, internal::PruneCompare<StateId, Weight>>;
if ((Weight::Properties() & kPath) != kPath) {
FSTERROR() << "Prune: Weight needs to have the path property: "
<< Arc::Weight::Type();
fst->SetProperties(kError, kError);
return;
}
auto ns = fst->NumStates();
if (ns < 1) return;
. . .
I do not understand why the code with enable_if is trying to help the compiler to compile one of the two code paths, but since the expression in the if() is pretty much constexpr (it should be, or it would not be allowed in enable_if anyway), the compiler must be totally insane to generate any code for the false branch, effectively producing the same template expansion.
Does my reasoning make sense, or am I missing something deeper?
As for this error message from your second attempt. this is certainly a bug in the compiler:
C2995: 'std::enable_if<,void>::type fst::Prune(const fst::Fst<Arc> &,fst::MutableFst<A> *,const fst::PruneOptions<Arc,ArcFilter> &)': function template has already been defined
std::enable_if<,void> cannot happen after expansion. Especially given the expression is constexpr bool.
yes, IIRC that was something they had before but for some reason, they changed to enable_if semantics (perhaps saving a couple of instructions?.
I wonder if that kPath test is really constexpr because it seems that it's both the condition and it's negation is evaluated as true, for which I don't see any than some deep compiler reasoning reasons. And additionally, why GCC doesn't mind? :)
I'll dig for the previous version of that file and try to get the preivous version working... y.
On Mon, Nov 6, 2017 at 4:42 PM, Kirill Katsnelson [email protected] wrote:
It looks like MSVC cannot digest that. Other compilers can.
But the deeper issue is why are they using enable_if; in this case it has no point.
I believe the best way to handle this is merge the two templates like
// Plus(weight, Weight::One()) == Weight::One()template <class Arc, class ArcFilter>void Prune(MutableFst<Arc> *fst, const PruneOptions<Arc, ArcFilter> &opts) { using StateId = typename Arc::StateId; using Weight = typename Arc::Weight; using StateHeap = Heap<StateId, internal::PruneCompare<StateId, Weight>>;
if ((Weight::Properties() & kPath) != kPath) { FSTERROR() << "Prune: Weight needs to have the path property: " << Arc::Weight::Type(); fst->SetProperties(kError, kError); return; }
auto ns = fst->NumStates(); if (ns < 1) return; . . .
I do not understand why the code with enable_if is trying to help the compiler to compile one of the two code paths, but since the expression in the if() is pretty much constexpr (it should be, or it would not be allowed in enable_if anyway), the compiler must be totally insane to generate any code for the false branch, effectively producing the same template expansion.
Does my reasoning make sense, or am I missing something deeper?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/6#issuecomment-342296896, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisXwSMsNTwPsS1du3otOw5ae9BV9R-ks5sz30ogaJpZM4QT2jT .
I wonder if that kPath test is really constexpr because it seems that it's both the condition and it's negation is evaluated as true
If you look closer at the error message, they both are evaluated as empty! enable_if<,void>. Complete nonsense.
oh, I wasn't able to parse that function prototype before (the "emptiness" of the expression). Now I see. y.
On Mon, Nov 6, 2017 at 4:53 PM, Kirill Katsnelson [email protected] wrote:
I wonder if that kPath test is really constexpr because it seems that it's both the condition and it's negation is evaluated as true
If you look closer at the error message, they both are evaluated as empty! enable_if<,void>. Complete nonsense.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/6#issuecomment-342300112, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisX-ePYnHJHQ72bfojJvWueMUB7ssoks5sz3_jgaJpZM4QT2jT .
IIRC that was something they had before but for some reason, they changed to enable_if semantics (perhaps saving a couple of instructions?.
That cannot save any this way, if anything goes as it should. Essentially, a compiler always compiles if (true) expr1; else expr2 as just expr1;. And enable_if can be used only with constexpr, as it is evaluated at the template expansion time.
The only reason I could think is they worked around some bug in a certain compiler?
I am getting this error in 3 headers from compiling this one file. Should not be a big patch. When you compile full fstscript, are there more than this?
I gave up when I had 5k errors, becuase the prune.h is included everywhere -- will keep working on it.
On Mon, Nov 6, 2017 at 5:01 PM, Kirill Katsnelson [email protected] wrote:
I am getting this error in 3 headers from compiling this one file. Should not be a big patch. When you compile full fstscript, are there more than this?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/6#issuecomment-342302292, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisXxA0adPMFyksFytzjEYUCseKehF6ks5sz4GtgaJpZM4QT2jT .
Let me come up with a "minimal" patch, so that it would be easy to maintain.
Looks like enable_if is used only in these 3 includes (the 4th use in util.h is rather valid).
OK, I'll wait. y.
On Mon, Nov 6, 2017 at 5:05 PM, Kirill Katsnelson [email protected] wrote:
Looks like enable_if is used only in these 3 includes (the 4th use in util.h is rather valid).
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/6#issuecomment-342303461, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisX5JGs9KySVATFuRX4iFe0C1duxJeks5sz4KhgaJpZM4QT2jT .
There. That file compiled. Please give the change a glance over if you have time: https://github.com/kkm000/openfst/commit/7b7db521175b956210a2ed197b408ef2ae224731?diff=unified
I went for half-indent of the if's to keep most code lines intact.
This is a new change in 1.6.5; here's a relevant comment from the old version:
// TODO(kbg): Make this a compile-time static_assert once:
// 1) All weight properties are made constexpr for all weight types.
// 2) We have a pleasant way to "deregister" this operation for non-path
// semirings so an informative error message is produced. The best
// solution will probably involve some kind of SFINAE magic.
So their idea is to produce a compile-time error instead of a runtime call to FSTERROR(). Now they are half-way there. Ok, let's see if the MS compiler catches up with this. I am not holding my breath tho. :(
I'm not sure if that's related to some of our issues, but apparently, even MSVC 17 doesn't support SFINAE completely: https://blogs.msdn.microsoft.com/vcblog/2017/08/11/c17-features-and-stl-fixes-in-vs-2017-15-3/ y.
On Mon, Nov 6, 2017 at 6:02 PM, Kirill Katsnelson [email protected] wrote:
This is a new change in 1.6.5; here's a relevant comment from the old version:
// TODO(kbg): Make this a compile-time static_assert once: // 1) All weight properties are made constexpr for all weight types. // 2) We have a pleasant way to "deregister" this operation for non-path // semirings so an informative error message is produced. The best // solution will probably involve some kind of SFINAE magic.
So their idea is to produce a compile-time error instead of a runtime call to FSTERROR(). Now they are half-way there. Ok, let's see if the MS compiler catches up with this. I am not holding my breath tho. :(
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/6#issuecomment-342317745, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisXzBMwSSK-RttyMBymcGAQ8vH_k6rks5sz5AdgaJpZM4QT2jT .
Thanks, quite an interesting post! I need to go through it carefully.
A very interesting note in the SO post that you sent me, exactly this answer about templates having to be distinguishable. It never occurred to me that it was a requirement. Now, this code fragment is accepted by icl, gcc and clang, but not MSVC:
#include <type_traits>
template <class T>
typename std::enable_if<T::SomeConstexprFunction() != 0>::type f() {
}
template <class T>
typename std::enable_if<T::SomeConstexprFunction() == 0>::type f() {
}
MSVC returns the familiar error
error C2995: 'std::enable_if<,void>::type f(void)': function template has already been defined
Now, I do not understand whether these templates are really "distinguishable" or not. Adding an extra parameter like template <class T, typename = void> fixes the problem for MSVC.
Also, I always thought that the use of the type was essential in a template, and it appears to be the case. In a sense, I understand the logic of the compiler: If a type is not used in the template, it can be evaluated immediately for a partial specialization. Where it breaks is the evaluation depends on other template arguments (which does not work, as the compiler wants to evaluate Weight::Properties() even though Weight is a parameter to the same template). So we are on a shaky ground here.
So in the end, the following pattern works (with the type of enable_if on the right side as a default argument type, which delays the evaluation until template expansion attempts) :
template <class Weight, typename = void,
typename = std::enable_if<
(Weight::Properties() & kIdempotent) == kIdempotent> >
bool WeightCompare(Weight w1, Weight w2, float delta, bool *error) {
return NaturalLess<Weight>()(w1, w2);
}
template <class Weight,
typename = std::enable_if<
((Weight::Properties() & kIdempotent) != kIdempotent)> >
bool WeightCompare(Weight w1, Weight w2, float delta, bool *error) {
// No natural order; use hash
. . .
But the extra typename = void is still required. And here we are looking at the scenario where the use of enable_if is probably not helpful (there is no potential compile-time error branch, if I got their intentions).
C++ templates are magic anyways, and I always prefer to leave magic to the magicians. :)
Do you think it makes sense to change the pattern now? Since it works now, I would rather leave it alone, and change to this pattern when (or if) they really start using SFINAE for compile-time errors.
EDIT: Added missing () in the code sample.
I have experimented with the additional parameter to the template as was described in the SO post. It compiled cleanly (there were some other probably unrelated issues), but what I got worried about is that using that approach we might be hiding the other template definition completely (as the enable_if always evaluates to the same, empty, results), so while it compiles, it wouldn't work as expected. But I didn't test it -- I'm, too, inclined to use the solution with the condition in the code. Every sane compiler should be able to optimize the extra code away.
y.
On Mon, Nov 6, 2017 at 8:21 PM, Kirill Katsnelson [email protected] wrote:
Thanks, quite an interesting post! I need to go through it carefully.
A very interesting note in the SO post that you sent me, exactly this answer https://stackoverflow.com/a/26753794/1149924 about templates having to be distinguishable. It never occurred to me that it was a requirement. Now, this code fragment is accepted by icl, gcc and clang, but not MSVC:
#include <type_traits> template <class T>typename std::enable_if<T::SomeConstexprFunction() != 0>::type f() { } template <class T>typename std::enable_if<T::SomeConstexprFunction() == 0>::type f() { }
MSVC returns the familiar error
error C2995: 'std::enable_if<,void>::type f(void)': function template has already been defined
Now, I do not understand whether these templates are really "distinguishable" or not. Adding an extra parameter like template <class T, typename = void> fixes the problem for MSVC.
Also, I always thought that the use of the type was essential in a template, and it appears to be the case. In a sense, I understand the logic of the compiler: If a type is not used in the template, it can be evaluated immediately for a partial specialization. Where it breaks is the evaluation depends on other template arguments (which does not work, as the compiler wants to evaluate Weight::Properties() even though Weight is a parameter to the same template). So we are on a shaky ground here.
So in the end, the following pattern works (with the type of enable_if on the right side as a default argument type, which delays the evaluation until template expansion attempts) :
template <class Weight, typename = void, typename = std::enable_if< (Weight::Properties() & kIdempotent) == kIdempotent> >bool WeightCompare(Weight w1, Weight w2, float delta, bool *error) { return NaturalLess<Weight>()(w1, w2); } template <class Weight, typename = std::enable_if< ((Weight::Properties & kIdempotent) != kIdempotent)> >bool WeightCompare(Weight w1, Weight w2, float delta, bool *error) { // No natural order; use hash . . .
But the extra typename = void is still required. And here we are looking at the scenario where the use of enable_if is probably not helpful (there is no potential compile-time error branch, if I got their intentions).
C++ templates are magic anyways, and I always prefer to leave magic to the magicians. :)
Do you think it makes sense to change the pattern now? Since it works now, I would rather leave it alone, and change to this pattern when (or if) they really start using SFINAE for compile-time errors.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/6#issuecomment-342344282, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisX_g3zpute9Po6fRRMmgftZd9Z9dUks5sz7CogaJpZM4QT2jT .
yes -- that's what I didn't understand -- if they want compile-time error, why not just static_assert? Anyway, I'd deal with that when it comes :) y.
On Mon, Nov 6, 2017 at 8:26 PM, Kirill Katsnelson [email protected] wrote:
Oh, and what they really want is probably just static_assert :)
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/6#issuecomment-342345123, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisX2e5r0tRFJZsh4DtpDYKmuPY7Tysks5sz7HBgaJpZM4QT2jT .
@jtrmal, I just committed the new version 1.6.7. I've hit this problem couple times more, as they added more template tests for properties. I think I found a better workaround for this bug, implemented here. IsIdempotent<T> and IsPath<T> are their new classes, I just rewrote them using my helper, which can be used for static property tests too.
I've created a new branch winport which is currently same as win/1.6. I'll keep them in sync for a while before retiring the latter, at least until I document the change (#3).
Changes kinda hard to review, but here's our cumulative set of changes as it stands: https://github.com/kkm000/openfst/compare/original...8b6ac5d