Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1665023002:
Cheer up spell-checking code (Closed)
|
Created:
4 years, 10 months ago by Kevin Bailey Modified:
4 years, 10 months ago CC:
chromium-reviews, groby+spellwatch_chromium.org, rlp+watch_chromium.org, rouslan+spell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheer up spell-checking code
Correctly format.
Convert raw pointers to scoped_ptr.
NULL -> nullptr
Remove some unnecessary std::set<> creation.
Committed: https://crrev.com/3621b07801552f5266fc899c5082fd11ffdb2d5a
Cr-Commit-Position: refs/heads/master@{#374656}
Patch Set 1 #
Total comments: 62
Patch Set 2 : Lots of clean up #Patch Set 3 : #Patch Set 4 : Restored formatting #Patch Set 5 : Missed one restore #Patch Set 6 : More clean up requests #Patch Set 7 : Removed set difference container #Patch Set 8 : Reverted one auto #
Total comments: 12
Patch Set 9 : More descriptive variable name #
Messages
Total messages: 21 (6 generated)
Description was changed from ========== Cheer up spell-checking code Correctly format. Convert raw pointers to scoped_ptr. NULL -> nullptr Remove some unnecessary std::set<> creation. ========== to ========== Cheer up spell-checking code Correctly format. Convert raw pointers to scoped_ptr. NULL -> nullptr Remove some unnecessary std::set<> creation. ==========
krb@chromium.org changed reviewers: + rouslan@chromium.org
Let me know if I missed anything; grep can only find so much.
Looks good. See comments and questions inline. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/feedback.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback.cc:24: #include "base/stl_util.h" No longer need this include. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback.cc:39: return NULL; Let's switch to nullptr in all of the files that you modify. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/feedback_sender.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:96: for (std::vector<Misspelling>::const_iterator suggestion_it = Let's switch to "for (const auto& suggestion : suggestions)" loop in this or later patch. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:132: return scoped_ptr<base::Value>(result.release()); Usually "return result;" gets the job done. If the compiler complains about conversion from DicitonaryValue to Value, try using "return result.Pass();" which does the upcasting for you. If ::Pass() is being deprecated (I do not recall), try "result std::move(result);" as the last resort. If none of these options work, fine, create an extra scoped_ptr :-) https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:187: if (!misspelling) return; Which clang-format are you using? I'm surprised it wants to do this. Perhaps it's not using the Chromium style, which is specified in https://code.google.com/p/chromium/codesearch#chromium/src/.clang-format https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:355: spellcheck::set_difference_container<std::vector<int>, std::set<int> > The ">>" is OK in C++11 now. No need to insert a space there anymore. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:356: renderers(known_renderers, alive_renderers); s/renderers/dead_renderers/ https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:357: for (auto renderer : renderers) { for (const auto& renderer : dead_renderers) { ... To make sure that "auto" does not attempt to create a copy or allow modifications. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/feedback_sender_unittest.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender_unittest.cc:185: #define SUGGESTIONINDEX 0 const int kSuggestionIndex = 0; if possible. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender_unittest.cc:187: #define STREVAL(x) STR(x) Let's put the "#define" statements before all of the namespaces to avoid thinking that namespaces affect scoping of pre-processor defines. (The don't.) https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/misspelling.h (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/misspelling.h:67: // Serializes the data in this object into a dictionary value. The caller owns "The caller owns the result." is not necessary, as scoped_ptr<> now implies it. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/set_difference_container.h (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:6: // without having to construct a set. This is more of a class comment, so let's move it above the class. A file comment would be useful to describe multiple, non-nested classes within a file. Also would be nice to provide a code snippet of sample usage in the class comment. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:14: class set_difference_container { Class names are usually CamelCase. Is there a precedent that you're following? https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:20: out_beg(container2.begin()), out_end(container2.end()) {} Delegate the construction to the second constructor for simplicity. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:22: set_difference_container( If you have a constructor, then you should have a destructor too, according to our style conventions. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:27: : in_beg(x_beg), in_end(x_end), out_beg(y_beg), out_end(y_end) {} Do something similar to https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&rc... to check that the data is sorted. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:29: class set_difference_iterator { Internal classes usually come before constructors. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:33: set_difference_iterator( Need a destructor too. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:53: set_difference_iterator operator++() { Return a reference to avoid a copy. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:54: // assert in_beg != in_end #include "base/logging.h" DCHECK_NE(in_beg, in_end); https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:71: private: newline above private. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:74: }; newline below }; https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:86: typename Container1::const_iterator in_beg, in_end; Each member variable on its own line, even if the type is the same. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:87: typename Container2::const_iterator out_beg, out_end; Ditto. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:101: std::set<std::string> > No space inside of ">>". https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:104: unsigned new_words_size = 0; size_t, since we're comparing it to std::set::size(), which returns a size_t. A size_t can be different from "unsigned int" on some platforms. For example, it could be "unsigned long". https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:453: words_.erase(word); Let's not switch this algorithm, because it's O(n*log(n)), as opposed to the old one, which is O(n).
groby@chromium.org changed reviewers: + groby@chromium.org
Please let's not do this. Including rouslan's suggestions, this CL now contains: * A wholesale reformatting. (And I'm not sure we need that at all) * Conversion of a few raw ptrs to scoped_ptrs * Comment changes * A simple search-and-replace change for NULL->nullptr * Introduction of an entire new (and untested) error structure to save construction of a fairly small temporary that gets constructed very rarely The motivation for this was the need to replace NULL->nullptr, which is a CL that's easily rubberstamped. Let's not complicate it by overloading it with a bunch of other things. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/feedback_sender_unittest.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender_unittest.cc:188: Let's just not do this. I can't see any value in these macros. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender_unittest.cc:306: 1, SpellCheckResult(SpellCheckResult::SPELLING, kMisspellingStart, If we're only reformatting all this code to make clang-format happy, let's not. As I pointed out on the other CL, "git cl format" restricts itself to modified lines, so formatting changes over time naturally. If we absolutely want the entire file clang-format compatible, I'd ask that that's the _only_ change in the CL, not additional fixes like the nullptr changes. (Fair warning: If we want that, I'll ask we'll do this for all of spellchecker, and we add a presubmit check for clang-format compatibility. :) https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/set_difference_container.h (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Do we have any reason to suspect a temporary set is so expensive that it justifies adding 90+ lines of code for a new container type?
Sorry, didn't read my email before hitting 'upload'. To answer a few questions: The CL was initiated before NULL -> nullptr came up. Rouslan and I had chatted about cheering up the code last week. The formatting/style updates were a low risk addition. Regarding removing the stl::set<>s, just to make sure I'm on the same page, these structures, while "temporary", are not pure stack structures. They will (completely needlessly) call malloc and free as much as any permanent structure. Nevertheless, is that a significant cost? I don't know but I can't imagine someone's custom dictionary or list of misspellings getting terribly big. However, I've seen more than a few times that no single blemish was enough for someone to generate a general solution, and just wanted to remove that hurdle. Anyways, I'm fine with abandoning the CL if you would prefer that, and will just have to get a better feel for what the team considers a significant enough benefit to outweigh the change (or just ask. :) )
So Rouslan was almost right: I was using the wrong clang-format, however the correct one found major differences in another file, so I just backed out all the formatting changes. I'm fine with making that a single CL or doing it gradually or whatever. I'm replying mostly for the sake of getting feedback on some items for future reviews. Let me know if any of the CL should be preserved. I'll remove the SetDifferenceContainer in the next round. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/feedback.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback.cc:24: #include "base/stl_util.h" On 2016/02/03 23:59:10, Rouslan wrote: > No longer need this include. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback.cc:39: return NULL; On 2016/02/03 23:59:10, Rouslan wrote: > Let's switch to nullptr in all of the files that you modify. Done (I think). https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/feedback_sender.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:96: for (std::vector<Misspelling>::const_iterator suggestion_it = On 2016/02/03 23:59:10, Rouslan wrote: > Let's switch to "for (const auto& suggestion : suggestions)" loop in this or > later patch. This was probably the most dangerous change, from a fat-finger perspective. I think I got them all except ones where it needed the iterator. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:132: return scoped_ptr<base::Value>(result.release()); On 2016/02/03 23:59:10, Rouslan wrote: > Usually "return result;" gets the job done. If the compiler complains about > conversion from DicitonaryValue to Value, try using "return result.Pass();" > which does the upcasting for you. If ::Pass() is being deprecated (I do not > recall), try "result std::move(result);" as the last resort. > > If none of these options work, fine, create an extra scoped_ptr :-) Ya, I was disappointed that it didn't automatically convert. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:187: if (!misspelling) return; On 2016/02/03 23:59:10, Rouslan wrote: > Which clang-format are you using? I'm surprised it wants to do this. Perhaps > it's not using the Chromium style, which is specified in > https://code.google.com/p/chromium/codesearch#chromium/src/.clang-format I'm glad you asked. depot_tools was at the end of my PATH, not beginning, but in the end, I'm not sure it made much difference. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:355: spellcheck::set_difference_container<std::vector<int>, std::set<int> > On 2016/02/03 23:59:10, Rouslan wrote: > The ">>" is OK in C++11 now. No need to insert a space there anymore. Ok, I thought it was "allowed", not "mandated". https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:356: renderers(known_renderers, alive_renderers); On 2016/02/03 23:59:10, Rouslan wrote: > s/renderers/dead_renderers/ Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender.cc:357: for (auto renderer : renderers) { On 2016/02/03 23:59:10, Rouslan wrote: > for (const auto& renderer : dead_renderers) { > ... > > To make sure that "auto" does not attempt to create a copy or allow > modifications. If you wish, but it's just an int, and auto is not allowed to enable modification. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/feedback_sender_unittest.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender_unittest.cc:185: #define SUGGESTIONINDEX 0 On 2016/02/03 23:59:10, Rouslan wrote: > const int kSuggestionIndex = 0; > > if possible. Rewritten. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender_unittest.cc:187: #define STREVAL(x) STR(x) On 2016/02/03 23:59:10, Rouslan wrote: > Let's put the "#define" statements before all of the namespaces to avoid > thinking that namespaces affect scoping of pre-processor defines. (The don't.) They're gone. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/feedback_sender_unittest.cc:188: On 2016/02/04 03:43:15, groby wrote: > Let's just not do this. I can't see any value in these macros. There was an honest to goodness bug in the original code. I was trying to fix it while preserving the author's intent to use the same symbol in both places. I've now changed it to use StringPrintf. Let me know if that's preferable (in the future). https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/misspelling.h (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/misspelling.h:67: // Serializes the data in this object into a dictionary value. The caller owns On 2016/02/03 23:59:10, Rouslan wrote: > "The caller owns the result." is not necessary, as scoped_ptr<> now implies it. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/set_difference_container.h (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/02/03 23:59:11, Rouslan wrote: > no (c) Done, and fixed in the file(s) that I copied it from. :) https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:6: // without having to construct a set. On 2016/02/03 23:59:11, Rouslan wrote: > This is more of a class comment, so let's move it above the class. A file > comment would be useful to describe multiple, non-nested classes within a file. > Also would be nice to provide a code snippet of sample usage in the class > comment. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:14: class set_difference_container { On 2016/02/03 23:59:10, Rouslan wrote: > Class names are usually CamelCase. Is there a precedent that you're following? Only standard C++ habit. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:20: out_beg(container2.begin()), out_end(container2.end()) {} On 2016/02/03 23:59:10, Rouslan wrote: > Delegate the construction to the second constructor for simplicity. Good idea. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:22: set_difference_container( On 2016/02/03 23:59:10, Rouslan wrote: > If you have a constructor, then you should have a destructor too, according to > our style conventions. I went through 3 style guides and the only thing mentioned about destructors was, of course, that you must have a virtual destructor in a virtual class, and that you don't want it in-line for a number of reasons, but nothing about mandating declaring it. Can you point me to this? https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:27: : in_beg(x_beg), in_end(x_end), out_beg(y_beg), out_end(y_end) {} On 2016/02/03 23:59:11, Rouslan wrote: > Do something similar to > https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&rc... > to check that the data is sorted. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:29: class set_difference_iterator { On 2016/02/03 23:59:11, Rouslan wrote: > Internal classes usually come before constructors. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:33: set_difference_iterator( On 2016/02/03 23:59:11, Rouslan wrote: > Need a destructor too. Acknowledged. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:53: set_difference_iterator operator++() { On 2016/02/03 23:59:10, Rouslan wrote: > Return a reference to avoid a copy. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:54: // assert in_beg != in_end On 2016/02/03 23:59:10, Rouslan wrote: > #include "base/logging.h" > > DCHECK_NE(in_beg, in_end); Sorry, didn't work, I suspect, because it was attempting to stream an iterator. If you know a way to fix that, happy to include it. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:71: private: On 2016/02/03 23:59:10, Rouslan wrote: > newline above private. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:74: }; On 2016/02/03 23:59:10, Rouslan wrote: > newline below }; Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:86: typename Container1::const_iterator in_beg, in_end; On 2016/02/03 23:59:10, Rouslan wrote: > Each member variable on its own line, even if the type is the same. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/set_difference_container.h:87: typename Container2::const_iterator out_beg, out_end; On 2016/02/03 23:59:10, Rouslan wrote: > Ditto. Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:101: std::set<std::string> > On 2016/02/03 23:59:11, Rouslan wrote: > No space inside of ">>". Done. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:104: unsigned new_words_size = 0; On 2016/02/03 23:59:11, Rouslan wrote: > size_t, since we're comparing it to std::set::size(), which returns a size_t. A > size_t can be different from "unsigned int" on some platforms. For example, it > could be "unsigned long". If true, the unsigned would be promoted. I think you're think of the dangers of comparing signed and unsigned. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:453: words_.erase(word); On 2016/02/03 23:59:11, Rouslan wrote: > Let's not switch this algorithm, because it's O(n*log(n)), as opposed to the old > one, which is O(n). The old one is very O(n ln(n)). :) Consider: line 445: go through ...to_add() and insert them in a set<> line 450: go through ...to_remove() and insert them in a new set<> line 449: assign to yet another set<>, but here you *might* get an RVO, or you might not and hit another O(n ln(n)) The new code is also n.ln(n) but at least doesn't create any new sets<>s, and IMHO is much more easy to read.
Re: std::set - this is so rare that we really don't care about the malloc overhead. If there truly is a need for a general solution, it belongs in base, right next to stl_util. (If you want to look at uses of STLSetDifference in Chromium and make the case for a general util, that is. base additions need proof they're actually needed) As a general philosophy, we're willing to trade a bit of performance in cold paths for reduced code size/complexity. Re: Abandoning or not - I think most of the changes made here _do_ improve the code base. I'd just advocate for breaking them up. Two reasons: 1) If a change can be made in a purely mechanical way, it's great to just upload that change, and the commands used to make the change. That way, the reviewer can focus on the change tool and then just spot-check. 2) Having unrelated changes in a single CL makes it harder when you do code archaeology 3 years from now. https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:104: unsigned new_words_size = 0; On 2016/02/04 16:34:12, Kevin Bailey wrote: > On 2016/02/03 23:59:11, Rouslan wrote: > > size_t, since we're comparing it to std::set::size(), which returns a size_t. > A > > size_t can be different from "unsigned int" on some platforms. For example, it > > could be "unsigned long". > > If true, the unsigned would be promoted. I think you're think of the dangers of > comparing signed and unsigned. We're trying to avoid implicit promotions. Quoth the Chromium style guide: Use size_t for object and allocation sizes, object counts, array and pointer offsets, vector indices, and so on https://codereview.chromium.org/1665023002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:453: words_.erase(word); Even if it were an O(n) vs O(n ln n) difference - and it isn't, STLSetDifference is a O(n ln n) because it inserts - it _really_ doesn't matter that much - because changes and dictionaries are usually pretty small. In terms of readability, I agree with Kevin - this is slightly better. IIRC we used STLSetDifference because pre-C++11, the for loop was a bit more hideous to read than this. As Bob Dylan says, the times, they are a-changing ;) So let's do Kevin's version - unless I suffer from E_LOW_COFFEE and am missing something critical? (And if we were _really_ performance-obsessed, we'd handcode a loop and use the hinting insert. But we really aren't. Really :) On 2016/02/04 16:34:12, Kevin Bailey wrote: > On 2016/02/03 23:59:11, Rouslan wrote: > > Let's not switch this algorithm, because it's O(n*log(n)), as opposed to the > old > > one, which is O(n). > > The old one is very O(n ln(n)). :) Consider: > > line 445: go through ...to_add() and insert them in a set<> > line 450: go through ...to_remove() and insert them in a new set<> > line 449: assign to yet another set<>, but here you *might* get an RVO, or you > might not and hit another O(n ln(n)) > > The new code is also n.ln(n) but at least doesn't create any new sets<>s, and > IMHO is much more easy to read.
I replaced the set difference container with more use of std::vector<> for the target container. It should at least reduce some n.ln(n) operations to n. Regarding the use of 'auto', I went through the style guides, pulled out these guidelines and tried to follow them: "Use auto to hide cumbersome names but prefer literal types for readability." So I specify things like int and uint32_t but not e.g. std::string or Misspelling. "Use 'auto x' when you want a copy. Use 'auto& x' when you want a reference." So for the containers of hashes (integers), I either used the actual type or 'auto', whichever looked better, and I used references everywhere else. Please let me know if any of them are at all offensive.
On 2016/02/04 21:39:00, Kevin Bailey wrote: > I replaced the set difference container with more use of std::vector<> for the > target container. It should at least reduce some n.ln(n) operations to n. > > Regarding the use of 'auto', I went through the style guides, pulled out these > guidelines and tried to follow them: > > "Use auto to hide cumbersome names but prefer literal types for readability." > > So I specify things like int and uint32_t but not e.g. std::string or > Misspelling. > > "Use 'auto x' when you want a copy. Use 'auto& x' when you want a reference." > > So for the containers of hashes (integers), I either used the actual type or > 'auto', whichever looked better, and I used references everywhere else. > > Please let me know if any of them are at all offensive. ping? We wanted this CL first, and it's not waiting on anything, ya?
LGTM % nits https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... File chrome/browser/spellchecker/feedback.cc (right): https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/feedback.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. Let's not touch the copyright stuff - see below. https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... File chrome/browser/spellchecker/feedback_sender.cc (right): https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/feedback_sender.cc:134: return std::move(result); Does this need to be std::move? The move ctor _should_ handle upcasts. https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/feedback_sender.cc:375: for (int renderer : dead_renderers) { nit: renderer_process_id (I wish we had a type for this, but since we don't, let's make clear what the int means) https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... File chrome/browser/spellchecker/feedback_sender.h (right): https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/feedback_sender.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. From the style guide: "we don't bother updating old headers". So let's not :) https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:338: if (i >= chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_WORDS) Since this is purely a size limiter, just make it i++ here. https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:498: ++i; See above.
https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... File chrome/browser/spellchecker/feedback.cc (right): https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/feedback.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/02/09 20:20:26, groby wrote: > Let's not touch the copyright stuff - see below. Done. https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... File chrome/browser/spellchecker/feedback_sender.cc (right): https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/feedback_sender.cc:134: return std::move(result); On 2016/02/09 20:20:26, groby wrote: > Does this need to be std::move? The move ctor _should_ handle upcasts. ../../chrome/browser/spellchecker/feedback_sender.cc:134:10: error: no viable conversion from returned value of type 'scoped_ptr<base::DictionaryValue>' to function return type 'scoped_ptr<base::Value>' return result; https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/feedback_sender.cc:375: for (int renderer : dead_renderers) { On 2016/02/09 20:20:26, groby wrote: > nit: renderer_process_id (I wish we had a type for this, but since we don't, > let's make clear what the int means) Done. https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... File chrome/browser/spellchecker/feedback_sender.h (right): https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/feedback_sender.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/02/09 20:20:26, groby wrote: > From the style guide: "we don't bother updating old headers". So let's not :) Done. https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:338: if (i >= chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_WORDS) On 2016/02/09 20:20:26, groby wrote: > Since this is purely a size limiter, just make it i++ here. Done. https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:498: ++i; On 2016/02/09 20:20:26, groby wrote: > See above. Done.
On 2016/02/09 22:26:39, Kevin Bailey wrote: > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > File chrome/browser/spellchecker/feedback.cc (right): > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > chrome/browser/spellchecker/feedback.cc:1: // Copyright 2013 The Chromium > Authors. All rights reserved. > On 2016/02/09 20:20:26, groby wrote: > > Let's not touch the copyright stuff - see below. > > Done. > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > File chrome/browser/spellchecker/feedback_sender.cc (right): > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > chrome/browser/spellchecker/feedback_sender.cc:134: return std::move(result); > On 2016/02/09 20:20:26, groby wrote: > > Does this need to be std::move? The move ctor _should_ handle upcasts. > > ../../chrome/browser/spellchecker/feedback_sender.cc:134:10: error: no viable > conversion from returned value of type 'scoped_ptr<base::DictionaryValue>' to > function return type 'scoped_ptr<base::Value>' > return result; > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > chrome/browser/spellchecker/feedback_sender.cc:375: for (int renderer : > dead_renderers) { > On 2016/02/09 20:20:26, groby wrote: > > nit: renderer_process_id (I wish we had a type for this, but since we don't, > > let's make clear what the int means) > > Done. > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > File chrome/browser/spellchecker/feedback_sender.h (right): > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > chrome/browser/spellchecker/feedback_sender.h:1: // Copyright 2013 The Chromium > Authors. All rights reserved. > On 2016/02/09 20:20:26, groby wrote: > > From the style guide: "we don't bother updating old headers". So let's not :) > > Done. > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:338: if (i >= > chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_WORDS) > On 2016/02/09 20:20:26, groby wrote: > > Since this is purely a size limiter, just make it i++ here. > > Done. > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:498: ++i; > On 2016/02/09 20:20:26, groby wrote: > > See above. > > Done. Ah, apologies. I meant to clear out the std::move message. A quick stroll through CS convinced me that we don't automatically upcast on return. :( (And then I hit "Send message" and forgot all about it)
On 2016/02/09 23:38:57, groby wrote: > On 2016/02/09 22:26:39, Kevin Bailey wrote: > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > File chrome/browser/spellchecker/feedback.cc (right): > > > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > chrome/browser/spellchecker/feedback.cc:1: // Copyright 2013 The Chromium > > Authors. All rights reserved. > > On 2016/02/09 20:20:26, groby wrote: > > > Let's not touch the copyright stuff - see below. > > > > Done. > > > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > File chrome/browser/spellchecker/feedback_sender.cc (right): > > > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > chrome/browser/spellchecker/feedback_sender.cc:134: return std::move(result); > > On 2016/02/09 20:20:26, groby wrote: > > > Does this need to be std::move? The move ctor _should_ handle upcasts. > > > > ../../chrome/browser/spellchecker/feedback_sender.cc:134:10: error: no viable > > conversion from returned value of type 'scoped_ptr<base::DictionaryValue>' to > > function return type 'scoped_ptr<base::Value>' > > return result; > > > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > chrome/browser/spellchecker/feedback_sender.cc:375: for (int renderer : > > dead_renderers) { > > On 2016/02/09 20:20:26, groby wrote: > > > nit: renderer_process_id (I wish we had a type for this, but since we don't, > > > let's make clear what the int means) > > > > Done. > > > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > File chrome/browser/spellchecker/feedback_sender.h (right): > > > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > chrome/browser/spellchecker/feedback_sender.h:1: // Copyright 2013 The > Chromium > > Authors. All rights reserved. > > On 2016/02/09 20:20:26, groby wrote: > > > From the style guide: "we don't bother updating old headers". So let's not > :) > > > > Done. > > > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right): > > > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:338: if (i >= > > chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_WORDS) > > On 2016/02/09 20:20:26, groby wrote: > > > Since this is purely a size limiter, just make it i++ here. > > > > Done. > > > > > https://codereview.chromium.org/1665023002/diff/140001/chrome/browser/spellch... > > chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:498: ++i; > > On 2016/02/09 20:20:26, groby wrote: > > > See above. > > > > Done. > > Ah, apologies. I meant to clear out the std::move message. A quick stroll > through CS convinced me that we don't automatically upcast on return. :( (And > then I hit "Send message" and forgot all about it) And still LGTM, FWIW
The CQ bit was checked by krb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665023002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665023002/160001
Message was sent while issue was closed.
Description was changed from ========== Cheer up spell-checking code Correctly format. Convert raw pointers to scoped_ptr. NULL -> nullptr Remove some unnecessary std::set<> creation. ========== to ========== Cheer up spell-checking code Correctly format. Convert raw pointers to scoped_ptr. NULL -> nullptr Remove some unnecessary std::set<> creation. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Cheer up spell-checking code Correctly format. Convert raw pointers to scoped_ptr. NULL -> nullptr Remove some unnecessary std::set<> creation. ========== to ========== Cheer up spell-checking code Correctly format. Convert raw pointers to scoped_ptr. NULL -> nullptr Remove some unnecessary std::set<> creation. Committed: https://crrev.com/3621b07801552f5266fc899c5082fd11ffdb2d5a Cr-Commit-Position: refs/heads/master@{#374656} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3621b07801552f5266fc899c5082fd11ffdb2d5a Cr-Commit-Position: refs/heads/master@{#374656} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
