Change "assert(false)" to "throw exception(...)" so it always fails, and also compiles properly with NDEBUG set #150
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nd also compiles properly with NDEBUG set.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
assert is replaced with nothing when compiling release (NDEBUG) code, breaking the compile (in MSVC at least) because the function doesn't return anything. Also, we really want this function to fail whether in debug or non-debug mode, so I think it's better to have assert(false) replaced by throw exception() instead. I don't know the proper exception for this situation.
@armatthews: Perhaps the number of invalid functions means the class hierarchy should be redesigned? It's a bit strange to have TreeLSTMBuilder be a RNNBuilder but have a bunch of functions that are not valid. Perhaps it could be:
class SomethingBuilder
class RNNBuilder : SomethingBuilder
class TreeLSTM: SomethingBuilder
Where the SomethingBuilder is just the subset of functions that are valid for treeLSTM also.
I don't fully understand why the functions aren't valid, so my opinion here might be way off.