Clean-up inline members of nested classes (base/)

base/values.h

 // Copyright (c) 2012 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.
 
 // This file specifies a recursive data storage class called Value intended for
 // storing setting and other persistable data.  It includes the ability to
 // specify (recursive) lists and dictionaries, so it's fairly expressive.
 // However, the API is optimized for the common case, namely storing a
 // hierarchical tree of simple values.  Given a DictionaryValue root, you can
 // easily do things like:
@@ skipping 322 matching lines @@
   // Swaps contents with the |other| dictionary.
   virtual void Swap(DictionaryValue* other);
 
   // This class provides an iterator for the keys in the dictionary.
   // It can't be used to modify the dictionary.
   //
   // YOU SHOULD ALWAYS USE THE XXXWithoutPathExpansion() APIs WITH THESE, NOT
   // THE NORMAL XXX() APIs.  This makes sure things will work correctly if any
   // keys have '.'s in them.
   class key_iterator
       : private std::iterator<std::input_iterator_tag, const std::string> {

[jar (doing other things), 2012/08/09 16:30:27]

Any idea why this is using private inheritance (violating style guide)?  Do you
know how this functionality is used at all in the class?

[hans, 2012/08/09 18:28:48]

No idea, I haven't really looked at the file itself, this is just a mechanical
change.

    public:
-    explicit key_iterator(ValueMap::const_iterator itr) { itr_ = itr; }
+    explicit key_iterator(ValueMap::const_iterator itr);
+    key_iterator(const key_iterator& rhs);

[jar (doing other things), 2012/08/09 16:30:27]

a) Why did you need to add this, if this CL is all about warnings?

b) Please add the explicit keyword.

[hans, 2012/08/09 18:28:48]

Because the plugin warns that this class needs an out-of-line copy constructor.

If we don't add one, the compiler will synthesize a default one for us, which is
inline, and the style guide tries to limit the amount of complex inline
functions in header files.
I don't think we do that for copy constructors.

[jar (doing other things), 2012/08/09 19:27:16]

You are correct about explicit. Thanks!

I think the style guide asks for a comment to that effect (noting why it is an
exception).




On 2012/08/09 18:28:48, hans wrote:

[hans, 2012/08/09 19:31:05]

Comment added.

     key_iterator operator++() {
       ++itr_;
       return *this;
     }
     const std::string& operator*() { return itr_->first; }
     bool operator!=(const key_iterator& other) { return itr_ != other.itr_; }
     bool operator==(const key_iterator& other) { return itr_ == other.itr_; }
 
    private:
     ValueMap::const_iterator itr_;
   };
 
   key_iterator begin_keys() const { return key_iterator(dictionary_.begin()); }
   key_iterator end_keys() const { return key_iterator(dictionary_.end()); }
 
   // This class provides an iterator over both keys and values in the
   // dictionary.  It can't be used to modify the dictionary.
   class Iterator {
    public:
-    explicit Iterator(const DictionaryValue& target)
-        : target_(target), it_(target.dictionary_.begin()) {}
+    explicit Iterator(const DictionaryValue& target);
 
     bool HasNext() const { return it_ != target_.dictionary_.end(); }
     void Advance() { ++it_; }
 
     const std::string& key() const { return it_->first; }
     const Value& value() const { return *it_->second; }
 
    private:
     const DictionaryValue& target_;
     ValueMap::const_iterator it_;
@@ skipping 129 matching lines @@
 
 }  // namespace base
 
 // http://crbug.com/88666
 using base::DictionaryValue;
 using base::ListValue;
 using base::StringValue;
 using base::Value;
 
 #endif  // BASE_VALUES_H_