-
Notifications
You must be signed in to change notification settings - Fork 271
Add const to query functions for cmd_ln_t and logmath_t #64
base: master
Are you sure you want to change the base?
Conversation
I'm sorry, Charles, is there any real value in this? I don't really see it. What about swig? It will not be happy I think. |
I submitted this because my own programming (in C++) makes heavy use of const correctness to keep track of ownership and thread safety. It feels very wrong to me to use a non-const pointer or reference to pass a parameter which will not be changed, to the point that my own code uses a number of Because adding the consts to the safe function signatures is free (it doesn't invalidate any code), it seemed like a good idea to add it to support those who wanted to pursue const correctness. The cost is that it makes the declarations more verbose and arguably harder to read, though personally I think it's easier to read because you can tell immediately whether or not the argument will be changed. A corollary of this is that it provides a hint about which pointers will be retained, though that is a whole other issue for thread safety which is probably beyond fixing. I admit I don't know much about swig; if you think it will be a problem I will check for issues and reassess whether or not I think this change is worthwhile. |
The biggest issue for me honestly. Sorry! |
If this were a more active development community I would suggest taking a poll to see if the consensus is that it's harder or easier, but in reality I suspect there would not be much participation! Assuming no other comments appear I will go back to using the |
@chussong interesting, what kind of C++ project are you working on using pocketsphinx? |
@nshmyrev We have an English-learning app which uses pocketsphinx as part of its pronunciation teaching module. All the processing is done on-device which means multithreading is important for modern devices, so I built a C++ wrapper around pocketsphinx in order to do threading while using the same code for Android and iOS. Over time I've had to replace large parts of pocketsphinx with my own C++ code in order to get thread safety and avoid duplicating resources, so there's also a lot of direct contact with sphinxbase as well. I've thought about using other libraries which would be a bit more naturally C++-friendly, but pocketsphinx has a small footprint and we don't yet have the resources to train our own small acoustic and language models, so we've stuck to the pocketsphinx base. And I've occasionally found small changes I'd like to make to Sphinx itself, so I've been submitting them as they come up in case others find them useful. |
Cool, send me the link, I'll try it! |
The website is here, but the app's target audience is Japanese secondary schools, so I doubt it will be of much interest! The pocketsphinx integration is also still under development on both platforms, and unfortunately the source code is proprietary so I probably can't say much more. Regarding const, I'll just point to this explanation in the C++ FAQ on why people use it and suggest that it would also be useful in the relatively object-oriented architecture of Sphinx, but since you don't like it I won't make any further comments or pull requests. (I had another one prepared for pocketsphinx but waited to see how this would be received before submitting it) If it's all right with you, I'd like to leave this PR open in case any developers come across it in the future and would like to weigh in on whether or not it would be worthwhile. |
Most of the functions in both of these classes can be
const
, allowing client code to be more careful about const correctness.Unfortunately the existence of the
refcount
member variable means that many applications will not be able to use these structs entirely asconst
, but this change makes it easier to pass them to functions without breaking anything.