CL for readability review

views/ime/character_composer.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "character_composer.h"
 
 #include <cstdlib>
 
 #include "third_party/gtk+/gdk/gdkkeysyms.h"
 
 namespace {
 
 inline int CompareSequenceValue(unsigned int key, uint16 value) {
   return (key > value) ? 1 : ((key < value) ? -1 : 0);
 }
 
 class ComposeChecker {
  public:
   ComposeChecker(const uint16* data, int max_sequence_length, int n_sequences);

[brettw, 2011/06/29 20:31:56]

Can you clarify the ownership of "data" in a comment here? It looks like you
require this to live as long as the class, which is something that should be
documented clearly.

[hashimoto, 2011/06/30 07:19:22]

Fixed

   bool CheckSequence(const std::vector<unsigned int>& sequence,
                      uint32* composed_character) const;
 
  private:
   static int CompareSequence(const void* key_void, const void* value_void);
 
   const uint16* data_;

[brettw, 2011/06/29 20:31:56]

I also like to make the ownership of member pointers clear. In this case, just a
"// Lifetime managed by class creator." or something would be sufficient (so
people don't think you need to delete it in the destructor).

[hashimoto, 2011/06/30 07:19:22]

Fixed

   int max_sequence_length_;
   int n_sequences_;
   int row_stride_;
 
   DISALLOW_COPY_AND_ASSIGN(ComposeChecker);
 };
 
 ComposeChecker::ComposeChecker(const uint16* data,
                                int max_sequence_length,
                                int n_sequences)
@@ skipping 30 matching lines @@
           found[max_sequence_length_ + 1];
       *composed_character = value;
     }
   }
   return true;
 }
 
 // static
 int ComposeChecker::CompareSequence(const void* key_void,
                                     const void* value_void) {
   typedef std::vector<unsigned int> KeyType;

[brettw, 2011/06/29 20:31:56]

This typedef appears 3 times, does it make sense to just make it a member of
ComposeChecker and use it in all 3 places? This may be unnecessary depending on
the answer to my bsearch question below.

[hashimoto, 2011/06/30 07:19:22]

typedef std::vector<unsigned int> ComposeBufferType;
is added to the top of the anonymous namespace

   const KeyType& key = *static_cast<const KeyType*>(key_void);
   const uint16* value = static_cast<const uint16*>(value_void);
 
   for(size_t i = 0; i < key.size(); ++i) {
     const int compare_result = CompareSequenceValue(key[i], value[i]);
     if(compare_result)
       return compare_result;
   }
   return 0;
 }
@@ skipping 47 matching lines @@
   // Check for composition sequences
   for (int length = compose_length - 1; length < max_sequence_length_;
        ++length) {
     const uint16* table = data_ + index[length];
     const uint16* table_next = data_ + index[length + 1];
     if (table_next > table) {
       // There are composition sequences for this |length|
       const int row_stride = length + 1;
       const int n_sequences = (table_next - table)/row_stride;
       const uint16* seq = static_cast<const uint16*>(
           bsearch(&sequence, table, n_sequences,

[brettw, 2011/06/29 20:31:56]

This bsearch stuff is kind of awkward because you don't have the typesafety and
need to do casts, etc. Did you consider std::lower_bound, which will operate on
the containers directly in a typesafe manner?

[hashimoto, 2011/06/30 07:19:22]

I use ugly bsearch instead of typesafe STL algorithms here because I have to
search on a two-dimensional array and coding style prohibits operator
overloading.
Using STL algorithms on two-dimensional array requires implementing customized
iterator, but I cannot implement iterators without operator overloading.

[brettw, 2011/07/01 22:21:18]

A raw pointer is actually a valid iterator for STL, we do this in several
places:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/ui/base/keycodes/keyb...
This code uses an operator< which, as you point out, it shouldn't do. But you
can pass a comparator functor as the 4th argument to lower_bound like this code
does:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/histor...

[hashimoto, 2011/07/04 04:45:20]

Yes, that's true about operator<, but I could not find a way to handle the
stride without operator++.
If I was allowed to use operator overloading, I would do something like below to
implement customized iterator (please forget about miscellaneous typedefs and
methods).

class MyIterator {
 public:
  MyIterator(uint16* ptr, int stride) : ptr(ptr), stride(stride) {}
  MyIterator& operator++() {
    ptr += stride;
    return *this;
  }
  uint16* operator*() {return ptr;}
 private:
  uint16* ptr;
  int stride;
};

                   sizeof(uint16)*row_stride, CompareSequenceSkipFront));
       if (seq) {
         if (length == compose_length - 1) // exact match
           *composed_character = seq[length];
         return true;
       }
     }
   }
   return false;
 }
@@ skipping 20 matching lines @@
   for(size_t i = 1; i < key.size(); ++i) {
     const int compare_result = CompareSequenceValue(key[i], value[i - 1]);
     if(compare_result)
       return compare_result;
   }
   return 0;
 }
 
 // Main table
 typedef uint16 guint16;
 #include "third_party/gtk+/gtk/gtkimcontextsimpleseqs.h"

[brettw, 2011/06/29 20:31:56]

It's not clear to me why this include is here in the middle of the file. Can it
be at the top with the regular ones? Does it need the typedef you provide?

[hashimoto, 2011/06/30 07:19:22]

Added #include "ui/base/gtk/gtk_integers.h" instead of typedef.
Added comment to describe why this file is included here.

 
 // Additional table
 
 // The difference between this and the default input method is the handling
 // of C+acute - this method produces C WITH CEDILLA rather than C WITH ACUTE.
 // For languages that use CCedilla and not acute, this is the preferred mapping,
 // and is particularly important for pt_BR, where the us-intl keyboard is
 // used extensively.
 
 const uint16 cedilla_compose_seqs[] = {
@@ skipping 32 matching lines @@
       return true;
     default:
       return false;
   }
 }
 
 bool CheckCharacterComposeTable(const std::vector<unsigned int>& sequence,
                                 uint32* composed_character) {
   // Check cedilla compose table
   const ComposeChecker kCedillaComposeChecker(
     cedilla_compose_seqs, 4, arraysize(cedilla_compose_seqs)/(4 + 2));

[brettw, 2011/06/29 20:31:56]

Wrapped lines should be indented 4 spaces.

[hashimoto, 2011/06/30 07:19:22]

Fixed

   if (kCedillaComposeChecker.CheckSequence(sequence, composed_character))
     return true;
 
   // Check main compose table
   const ComposeCheckerWithCompactTable kMainComposeChecker(
       gtk_compose_seqs_compact, 5, 24, 6);
   if (kMainComposeChecker.CheckSequence(sequence, composed_character))
     return true;
 
   return false;
 }
 
 }  // anonymous namespace

[brettw, 2011/06/29 20:31:56]

You don't need the "anonymous" here, just "// namespace"
http://www/eng/doc/cppguide.xml?external=y&showone=Namespace_Formatting#Names...

[hashimoto, 2011/06/30 07:19:22]

Fixed all two

 
 namespace views {
 
 CharacterComposer::CharacterComposer() {}
 
 CharacterComposer::~CharacterComposer() {}
 
 void CharacterComposer::Reset() {
   compose_buffer_.clear();
   composed_character_.clear();
@@ skipping 13 matching lines @@
     if (composed_character_utf32 !=0) {
       // We get a composed character
       compose_buffer_.clear();
       // We assume that composed character is in BMP
       if (composed_character_utf32 <= 0xffff)
         composed_character_ += static_cast<char16>(composed_character_utf32);
     }
     return true;
   }
   // Key press is not a part of composition
   compose_buffer_.pop_back(); // remove the keypress added this time

[brettw, 2011/06/29 20:31:56]

Please be sure all your comments begin with a capital letter and end with a
period:
http://www/eng/doc/cppguide.xml?external=y&showone=Punctuation,_Spelling_and_...

Also, in this case, end-of-line comments should have two spaces before the "//"

[hashimoto, 2011/06/30 07:19:22]

Fixed

   if (!compose_buffer_.empty()) {
     compose_buffer_.clear();
     return true;
   }
   return false;
 }
 
 }  // namespace views
+//

[brettw, 2011/06/29 20:31:56]

Be sure to delete this once you've made other changes to this file.

[hashimoto, 2011/06/30 07:19:22]

Deleted all three.