#132 Change C++ port public API #146
Conversation
Signed-off-by: vityaman <[email protected]>
Signed-off-by: vityaman <[email protected]>
vityaman
force-pushed
the
132-port-cpp-change-api
branch
from
92776de to
de54895
Compare
| void CodeCompletionCore::setIgnoredTokens(std::unordered_set<size_t>&& ignoredTokens) { | ||
| this->ignoredTokens = std::move(ignoredTokens); | ||
| } | ||
|
|
||
| void CodeCompletionCore::setPreferredRules(std::unordered_set<size_t>&& preferredRules) { | ||
| this->preferredRules = std::move(preferredRules); | ||
| } | ||
|
|
||
| void CodeCompletionCore::setTranslateRulesTopDown(bool isEnabled) { | ||
| this->translateRulesTopDown = isEnabled; | ||
| } |
There was a problem hiding this comment.
What's the purpose of these setters? Since there's no side processing when setting a value, there's no need to use a setter.
|
|
||
| if (showResult) { | ||
| if (candidates.cancelled) { | ||
| if (show.result) { |
There was a problem hiding this comment.
show.result? This is a single flag named showResult. Why splitting it to an extent that becomes no longer easy to understand?
| @@ -267,7 +279,7 @@ bool CodeCompletionCore::translateToRuleIndex( | |||
| .startTokenIndex = rwst.startTokenIndex, | |||
| .ruleList = path, | |||
| }; | |||
| if (showDebugOutput) { | |||
| if (show.debugOutput) { | |||
| @@ -110,44 +129,49 @@ class CodeCompletionCore { | |||
| * Tailoring of the result: | |||
| * Tokens which should not appear in the candidates set. | |||
| */ | |||
| std::unordered_set<size_t> ignoredTokens; // NOLINT: public field | |||
| void setIgnoredTokens(std::unordered_set<size_t>&& ignoredTokens); | |||
There was a problem hiding this comment.
I see no reason why a public field could be harmful in any way, if it has no side effect when setting it. Additionally, it's a result field, so it is supposed to be a simple public field!
| * Enables call stack printing for each rule recursion. | ||
| */ | ||
| bool ruleStack = false; | ||
| } show; // NOLINT: public field, only for debugging |
There was a problem hiding this comment.
Putting all debug options in an own struct is ok, just don't name it show. Additionally type names have to be in PascalCase, so a natural choice here would be: DebugOptions instead of show and it would be passed in as parameter like (..., DebugOptions const& debugOptions)...
ports/cpp/test/cpp14/Cpp14Test.cpp
Outdated
| @@ -38,7 +38,7 @@ TEST(CPP14Parser, SimpleExample) { // NOLINT: complexity | |||
| CodeCompletionCore completion(&pipeline.parser); | |||
|
|
|||
| // Ignore operators and the generic ID token. | |||
| completion.ignoredTokens = { | |||
| completion.setIgnoredTokens({ | |||
There was a problem hiding this comment.
Using a name like set<anything> without a get<anything> makes this look as if it were write only. Again, I see no need to make this a setter. It's a simple configuration step, why make it complicated? One could pass that in as a parameter to the main collection call, but since this is something configured once for a specific parser, the simple assignment is more appropriate.
I believe the term "parameter pack" is used in the context of type parameters, but I know what you mean.
Unnecessary. If you change the storage type (which is really uncritical, both in time and space) let the user know and they can adapt. |
Signed-off-by: vityaman <[email protected]>
Signed-off-by: vityaman <[email protected]>
Signed-off-by: vityaman <[email protected]>
vityaman
force-pushed
the
132-port-cpp-change-api
branch
from
de54895 to
7a6c202
Compare
Signed-off-by: vityaman <[email protected]>
vityaman
force-pushed
the
132-port-cpp-change-api
branch
from
7a6c202 to
5b276d5
Compare
|
Introduced an optional parameters pack for
collectCandidates. Because current API is not so convenient for usage of only a part of arguments.Did hide
preferredRulesandignoredTokensunder setters. This will provide us an ability to change internal data structures to improve performance (I'm thinking about usingBitSet, because it is more cache friendly while there are not, so many token types in average generated parser).Renamed some variables.
Raised port version to
2.0.0because of API breaking changes.