Skip to content
This repository has been archived by the owner on Jun 9, 2022. It is now read-only.

Add const to query functions for cmd_ln_t and logmath_t #64

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chussong
Copy link
Contributor

@chussong chussong commented Apr 9, 2019

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 as const, but this change makes it easier to pass them to functions without breaking anything.

@nshmyrev
Copy link
Contributor

nshmyrev commented Apr 9, 2019

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.

@chussong
Copy link
Contributor Author

chussong commented Apr 9, 2019

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 const_casts to remove const at the call site when I can be sure it's safe.

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.

@nshmyrev
Copy link
Contributor

nshmyrev commented Apr 9, 2019

The cost is that it makes the declarations more verbose and arguably harder to read

The biggest issue for me honestly. Sorry!

@chussong
Copy link
Contributor Author

chussong commented Apr 9, 2019

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 const_casts in my interfacing code.

@nshmyrev
Copy link
Contributor

nshmyrev commented Apr 9, 2019

@chussong interesting, what kind of C++ project are you working on using pocketsphinx?

@chussong
Copy link
Contributor Author

chussong commented Apr 9, 2019

@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.

@nshmyrev
Copy link
Contributor

nshmyrev commented Apr 9, 2019

Cool, send me the link, I'll try it!

@chussong
Copy link
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants