Skip to content

Commit

Permalink
More removals of GET_CONFIG.
Browse files Browse the repository at this point in the history 
This CL is a preparation of Singleton<Config> removal.

GET_CONFIG relies on process global Singleton<Config>, which has caused
many troubles.  With this CL, many conversion-related methods will be
refactored to receive ConversionRequest to access settings.

Hopefully this can help to address or at least mitigate test flakiness
like #317.

BUG=#317
TEST=unittest
REF_BUG=19010851,19188911
REF_CL=87318105
  • Loading branch information
@hiroyuki-komatsu @yukawa
hiroyuki-komatsu authored and yukawa committed Nov 9, 2015
1 parent 8ab3383 commit fa02514
Show file tree
Hide file tree
Showing 31 changed files with 514 additions and 345 deletions.
27 changes: 8 additions & 19 deletions src/converter/immutable_converter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,7 @@ void InsertCorrectedNodes(size_t pos, const string &key,
}
KeyCorrectedNodeListBuilder builder(pos, key, key_corrector,
lattice->node_allocator());
dictionary->LookupPrefix(
StringPiece(str, length),
request.IsKanaModifierInsensitiveConversion(),
&builder);
dictionary->LookupPrefix(StringPiece(str, length), request, &builder);
if (builder.tail() != NULL) {
builder.tail()->bnext = NULL;
}
Expand Down Expand Up @@ -776,28 +773,22 @@ Node *ImmutableConverterImpl::Lookup(const int begin_pos,
BaseNodeListBuilder builder(
lattice->node_allocator(),
lattice->node_allocator()->max_nodes_size());
dictionary_->LookupReverse(StringPiece(begin, len), &builder);
dictionary_->LookupReverse(StringPiece(begin, len), request, &builder);
result_node = builder.result();
} else {
if (is_prediction) {
NodeListBuilderWithCacheEnabled builder(
lattice->node_allocator(),
lattice->cache_info(begin_pos) + 1);
dictionary_->LookupPrefix(
StringPiece(begin, len),
request.IsKanaModifierInsensitiveConversion(),
&builder);
dictionary_->LookupPrefix(StringPiece(begin, len), request, &builder);
result_node = builder.result();
lattice->SetCacheInfo(begin_pos, len);
} else {
// When cache feature is not used, look up normally
BaseNodeListBuilder builder(
lattice->node_allocator(),
lattice->node_allocator()->max_nodes_size());
dictionary_->LookupPrefix(
StringPiece(begin, len),
request.IsKanaModifierInsensitiveConversion(),
&builder);
dictionary_->LookupPrefix(StringPiece(begin, len), request, &builder);
result_node = builder.result();
}
}
Expand Down Expand Up @@ -1104,7 +1095,7 @@ void ImmutableConverterImpl::PredictionViterbiInternal(
// Note that, the average number of lid/rid variation is less than 30 in
// most cases. So, in order to avoid too many allocations for internal
// nodes of std::map, we use vector of key-value pairs.
typedef vector<pair<int, pair<int, Node*> > > BestMap;
typedef vector<pair<int, pair<int, Node*>>> BestMap;
typedef OrderBy<FirstKey, Less> OrderByFirst;
BestMap lbest, rbest;
lbest.reserve(128);
Expand Down Expand Up @@ -1267,8 +1258,7 @@ void ImmutableConverterImpl::MakeLatticeNodesForPredictiveNodes(
lattice->node_allocator()->max_nodes_size(),
pos_matcher_);
suffix_dictionary_->LookupPredictive(
StringPiece(key.data() + pos, key.size() - pos),
request.IsKanaModifierInsensitiveConversion(), &builder);
StringPiece(key.data() + pos, key.size() - pos), request, &builder);
if (builder.result() != NULL) {
lattice->Insert(pos, builder.result());
}
Expand Down Expand Up @@ -1298,8 +1288,7 @@ void ImmutableConverterImpl::MakeLatticeNodesForPredictiveNodes(
lattice->node_allocator()->max_nodes_size(),
pos_matcher_);
dictionary_->LookupPredictive(
StringPiece(key.data() + pos, key.size() - pos),
request.IsKanaModifierInsensitiveConversion(), &builder);
StringPiece(key.data() + pos, key.size() - pos), request, &builder);
if (builder.result() != NULL) {
lattice->Insert(pos, builder.result());
}
Expand Down Expand Up @@ -1584,7 +1573,7 @@ void ImmutableConverterImpl::MakeLatticeNodesForConversionSegments(
std::unique_ptr<KeyCorrector> key_corrector;
if (is_conversion && !segments.resized()) {
KeyCorrector::InputMode mode = KeyCorrector::ROMAN;
if (GET_CONFIG(preedit_method) != config::Config::ROMAN) {
if (request.config().preedit_method() != config::Config::ROMAN) {
mode = KeyCorrector::KANA;
}
key_corrector.reset(new KeyCorrector(key, mode, history_key.size()));
Expand Down
67 changes: 30 additions & 37 deletions src/converter/immutable_converter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,23 +176,7 @@ class MockDataAndImmutableConverter {

} // namespace

class ImmutableConverterTest : public ::testing::Test {
protected:
virtual void SetUp() {
SystemUtil::SetUserProfileDirectory(FLAGS_test_tmpdir);
config::ConfigHandler::GetDefaultConfig(&default_config_);
config::ConfigHandler::SetConfig(default_config_);
}

virtual void TearDown() {
config::ConfigHandler::SetConfig(default_config_);
}

private:
config::Config default_config_;
};

TEST_F(ImmutableConverterTest, KeepKeyForPrediction) {
TEST(ImmutableConverterTest, KeepKeyForPrediction) {
std::unique_ptr<MockDataAndImmutableConverter> data_and_converter(
new MockDataAndImmutableConverter);
Segments segments;
Expand All @@ -210,7 +194,7 @@ TEST_F(ImmutableConverterTest, KeepKeyForPrediction) {
EXPECT_EQ(kRequestKey, segments.segment(0).key());
}

TEST_F(ImmutableConverterTest, DummyCandidatesCost) {
TEST(ImmutableConverterTest, DummyCandidatesCost) {
std::unique_ptr<MockDataAndImmutableConverter> data_and_converter(
new MockDataAndImmutableConverter);
Segment segment;
Expand All @@ -222,7 +206,7 @@ TEST_F(ImmutableConverterTest, DummyCandidatesCost) {
EXPECT_LT(segment.candidate(0).wcost, segment.candidate(2).wcost);
}

TEST_F(ImmutableConverterTest, DummyCandidatesInnerSegmentBoundary) {
TEST(ImmutableConverterTest, DummyCandidatesInnerSegmentBoundary) {
std::unique_ptr<MockDataAndImmutableConverter> data_and_converter(
new MockDataAndImmutableConverter);
Segment segment;
Expand Down Expand Up @@ -252,7 +236,8 @@ class KeyCheckDictionary : public DictionaryInterface {
virtual bool HasValue(StringPiece value) const { return false; }

virtual void LookupPredictive(
StringPiece key, bool use_kana_modifier_insensitive_looukp,
StringPiece key,
const ConversionRequest &convreq,
Callback *callback) const {
if (key == target_query_) {
received_target_query_ = true;
Expand All @@ -261,16 +246,20 @@ class KeyCheckDictionary : public DictionaryInterface {

virtual void LookupPrefix(
StringPiece key,
bool use_kana_modifier_insensitive_looukp,
const ConversionRequest &convreq,
Callback *callback) const {
// No check
}

virtual void LookupExact(StringPiece key, Callback *callback) const {
virtual void LookupExact(StringPiece key,
const ConversionRequest &convreq,
Callback *callback) const {
// No check
}

virtual void LookupReverse(StringPiece str, Callback *callback) const {
virtual void LookupReverse(StringPiece str,
const ConversionRequest &convreq,
Callback *callback) const {
// No check
}

Expand All @@ -284,7 +273,7 @@ class KeyCheckDictionary : public DictionaryInterface {
};
} // namespace

TEST_F(ImmutableConverterTest, PredictiveNodesOnlyForConversionKey) {
TEST(ImmutableConverterTest, PredictiveNodesOnlyForConversionKey) {
Segments segments;
{
Segment *segment = segments.add_segment();
Expand Down Expand Up @@ -327,7 +316,7 @@ TEST_F(ImmutableConverterTest, PredictiveNodesOnlyForConversionKey) {
EXPECT_FALSE(dictionary->received_target_query());
}

TEST_F(ImmutableConverterTest, AddPredictiveNodes) {
TEST(ImmutableConverterTest, AddPredictiveNodes) {
Segments segments;
{
Segment *segment = segments.add_segment();
Expand Down Expand Up @@ -356,7 +345,7 @@ TEST_F(ImmutableConverterTest, AddPredictiveNodes) {
EXPECT_TRUE(dictionary->received_target_query());
}

TEST_F(ImmutableConverterTest, InnerSegmenBoundaryForPrediction) {
TEST(ImmutableConverterTest, InnerSegmenBoundaryForPrediction) {
std::unique_ptr<MockDataAndImmutableConverter> data_and_converter(
new MockDataAndImmutableConverter);
Segments segments;
Expand Down Expand Up @@ -409,7 +398,7 @@ TEST_F(ImmutableConverterTest, InnerSegmenBoundaryForPrediction) {
EXPECT_EQ("\xe4\xb8\xad\xe3\x83\x8e", content_values[2]);
}

TEST_F(ImmutableConverterTest, NoInnerSegmenBoundaryForConversion) {
TEST(ImmutableConverterTest, NoInnerSegmenBoundaryForConversion) {
std::unique_ptr<MockDataAndImmutableConverter> data_and_converter(
new MockDataAndImmutableConverter);
Segments segments;
Expand All @@ -430,7 +419,7 @@ TEST_F(ImmutableConverterTest, NoInnerSegmenBoundaryForConversion) {
}
}

TEST_F(ImmutableConverterTest, NotConnectedTest) {
TEST(ImmutableConverterTest, NotConnectedTest) {
std::unique_ptr<MockDataAndImmutableConverter> data_and_converter(
new MockDataAndImmutableConverter);
ImmutableConverterImpl *converter = data_and_converter->GetConverter();
Expand Down Expand Up @@ -478,7 +467,7 @@ TEST_F(ImmutableConverterTest, NotConnectedTest) {
EXPECT_TRUE(tested);
}

TEST_F(ImmutableConverterTest, HistoryKeyLengthIsVeryLong) {
TEST(ImmutableConverterTest, HistoryKeyLengthIsVeryLong) {
// "あ..." (100 times)
const string kA100 =
"\xE3\x81\x82\xE3\x81\x82\xE3\x81\x82\xE3\x81\x82\xE3\x81\x82"
Expand Down Expand Up @@ -563,32 +552,36 @@ bool AutoPartialSuggestionTestHelper(const ConversionRequest &request) {
}
} // namespace

TEST_F(ImmutableConverterTest, EnableAutoPartialSuggestion) {
TEST(ImmutableConverterTest, EnableAutoPartialSuggestion) {
const commands::Request request;
ConversionRequest conversion_request(NULL, &request);
ConversionRequest conversion_request;
conversion_request.set_request(&request);
conversion_request.set_create_partial_candidates(true);

EXPECT_TRUE(AutoPartialSuggestionTestHelper(conversion_request));
}

TEST_F(ImmutableConverterTest, DisableAutoPartialSuggestion) {
TEST(ImmutableConverterTest, DisableAutoPartialSuggestion) {
const commands::Request request;
ConversionRequest conversion_request(NULL, &request);
ConversionRequest conversion_request;
conversion_request.set_request(&request);
conversion_request.set_create_partial_candidates(false);

EXPECT_FALSE(AutoPartialSuggestionTestHelper(conversion_request));
}

TEST_F(ImmutableConverterTest, AutoPartialSuggestionDefault) {
TEST(ImmutableConverterTest, AutoPartialSuggestionDefault) {
const commands::Request request;
ConversionRequest conversion_request(NULL, &request);
ConversionRequest conversion_request;
conversion_request.set_request(&request);

EXPECT_FALSE(AutoPartialSuggestionTestHelper(conversion_request));
}

TEST_F(ImmutableConverterTest, AutoPartialSuggestionForSingleSegment) {
TEST(ImmutableConverterTest, AutoPartialSuggestionForSingleSegment) {
const commands::Request request;
ConversionRequest conversion_request(NULL, &request);
ConversionRequest conversion_request;
conversion_request.set_request(&request);
conversion_request.set_create_partial_candidates(true);

std::unique_ptr<MockDataAndImmutableConverter> data_and_converter(
Expand Down
55 changes: 32 additions & 23 deletions src/dictionary/dictionary_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
#include "base/logging.h"
#include "base/string_piece.h"
#include "base/util.h"
#include "config/config_handler.h"
#include "dictionary/dictionary_interface.h"
#include "dictionary/dictionary_token.h"
#include "dictionary/pos_matcher.h"
Expand Down Expand Up @@ -147,72 +146,82 @@ class CallbackWithFilter : public DictionaryInterface::Callback {

void DictionaryImpl::LookupPredictive(
StringPiece key,
bool use_kana_modifier_insensitive_lookup,
const ConversionRequest &conversion_request,
Callback *callback) const {
CallbackWithFilter callback_with_filter(
GET_CONFIG(use_spelling_correction),
GET_CONFIG(use_zip_code_conversion),
GET_CONFIG(use_t13n_conversion),
conversion_request.config().use_spelling_correction(),
conversion_request.config().use_zip_code_conversion(),
conversion_request.config().use_t13n_conversion(),
pos_matcher_,
suppression_dictionary_,
callback);
for (size_t i = 0; i < dics_.size(); ++i) {
dics_[i]->LookupPredictive(
key, use_kana_modifier_insensitive_lookup, &callback_with_filter);
key,
conversion_request,
&callback_with_filter);
}
}

void DictionaryImpl::LookupPrefix(
StringPiece key,
bool use_kana_modifier_insensitive_lookup,
const ConversionRequest &conversion_request,
Callback *callback) const {
CallbackWithFilter callback_with_filter(
GET_CONFIG(use_spelling_correction),
GET_CONFIG(use_zip_code_conversion),
GET_CONFIG(use_t13n_conversion),
conversion_request.config().use_spelling_correction(),
conversion_request.config().use_zip_code_conversion(),
conversion_request.config().use_t13n_conversion(),
pos_matcher_,
suppression_dictionary_,
callback);
for (size_t i = 0; i < dics_.size(); ++i) {
dics_[i]->LookupPrefix(
key, use_kana_modifier_insensitive_lookup, &callback_with_filter);
key,
conversion_request,
&callback_with_filter);
}
}

void DictionaryImpl::LookupExact(StringPiece key, Callback *callback) const {
void DictionaryImpl::LookupExact(
StringPiece key,
const ConversionRequest &conversion_request,
Callback *callback) const {
CallbackWithFilter callback_with_filter(
GET_CONFIG(use_spelling_correction),
GET_CONFIG(use_zip_code_conversion),
GET_CONFIG(use_t13n_conversion),
conversion_request.config().use_spelling_correction(),
conversion_request.config().use_zip_code_conversion(),
conversion_request.config().use_t13n_conversion(),
pos_matcher_,
suppression_dictionary_,
callback);
for (size_t i = 0; i < dics_.size(); ++i) {
dics_[i]->LookupExact(key, &callback_with_filter);
dics_[i]->LookupExact(key, conversion_request, &callback_with_filter);
}
}

void DictionaryImpl::LookupReverse(StringPiece str,
Callback *callback) const {
void DictionaryImpl::LookupReverse(
StringPiece str,
const ConversionRequest &conversion_request,
Callback *callback) const {
CallbackWithFilter callback_with_filter(
GET_CONFIG(use_spelling_correction),
GET_CONFIG(use_zip_code_conversion),
GET_CONFIG(use_t13n_conversion),
conversion_request.config().use_spelling_correction(),
conversion_request.config().use_zip_code_conversion(),
conversion_request.config().use_t13n_conversion(),
pos_matcher_,
suppression_dictionary_,
callback);
for (size_t i = 0; i < dics_.size(); ++i) {
dics_[i]->LookupReverse(str, &callback_with_filter);
dics_[i]->LookupReverse(str, conversion_request, &callback_with_filter);
}
}

bool DictionaryImpl::LookupComment(StringPiece key, StringPiece value,
const ConversionRequest &conversion_request,
string *comment) const {
// TODO(komatsu): UserDictionary should be treated as the highest priority.
// In the current implementation, UserDictionary is the last node of dics_,
// but the only dictionary which may return true.
for (size_t i = 0; i < dics_.size(); ++i) {
if (dics_[i]->LookupComment(key, value, comment)) {
if (dics_[i]->LookupComment(key, value, conversion_request, comment)) {
return true;
}
}
Expand Down
Loading

0 comments on commit fa02514

Please sign in to comment.