Issue 568002: Rewrite SimpleStringDictionary with NonAllocatingMap. (Closed)
|
Created:
11 years ago by rsesek Modified:
11 years ago Reviewers:
Mark Mentovai CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionRewrite SimpleStringDictionary with NonAllocatingMap. NonAllocatingMap has a near-identical interface, but is significantly less code, more customizable, and has storage that is POD. BUG=http://code.google.com/p/chromium/issues/detail?id=77656 Committed: https://code.google.com/p/google-breakpad/source/detail?r=1161 Patch Set 1 #
Total comments: 50
Patch Set 2 : #
Total comments: 20
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
MessagesTotal messages: 10
SimpleStringDictionary's KeyValueEntry not being POD meant I couldn't reuse this the way I wanted to… so I aggressively cleaned it up. Feel free to suggest a better name than NonAllocatingMap.
Sign in to reply to this message.
https://breakpad.appspot.com/568002/diff/1/src/client/mac/crash_generation/In... File src/client/mac/crash_generation/Inspector.h (right): https://breakpad.appspot.com/568002/diff/1/src/client/mac/crash_generation/In... src/client/mac/crash_generation/Inspector.h:68: KeyValueMessageData(const google_breakpad::SimpleStringDictionary::Entry &inEntry) { Mark this constructor explicit AND fix its 80-columness. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionary.h File src/common/simple_string_dictionary.h (right): https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:33: #include <cassert> <assert.h>, not <cassert>. And then take out the blank line between this and <string.h> (If you were using a C++ header, the correct order would have been after <string.h>.) https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:50: // and includes space for the NULL byte. This gives space for KeySize-1 and include, not includes NULL is a pointer value, not a byte value. NUL or '\0' is a byte value. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:51: // ValueSize-1 characters in an entry. Since the effective limits are KeySize-1 and ValueSize-1 but NumEntries (not NumEntries-1) for the entries parameter, you might want to say so. Unless you really do need room for a NULL entry, in which case, you should say that. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:53: class NonAllocatingMap { non_allocating_map.h? https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:55: // The total amount of space the entry_map() is. What’s an entry_map()? https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:56: static const size_t total_size = (KeySize + ValueSize) * NumEntries; I’d put this after the three other static const size_ts, just because it’s kind of more sensible that way when you’re reading from top to bottom. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:59: static const size_t key_size = KeySize; Do you want to name your constants like kKeySize, etc., so that it’s obvious that they’re constants? Or do you hate Hungarians? https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:103: memset(&entries_, 0, sizeof(entries_)); Just writing an initializer list of entries_() is sufficient to zero out entries_. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:106: NonAllocatingMap(const NonAllocatingMap& other) { If it doesn’t need to be copyable and assignable, I’d get rid of this constructor and the assignment operator. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:111: assert(other.total_size == total_size); You want to check more than the total size, you want to make sure that the keys and values are the same size too. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:111: assert(other.total_size == total_size); Elsewhere, following an assert, you have a check for the same condition the assert is looking for, to prevent the code from doing anything too evil in assertion-free release builds. You should consider doing that here too. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:137: if (entry.is_active() && strcmp(entry.key, key) == 0) { Use strNcmp for added safety since you have the maximum length handy in a constant anyway. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:146: // must not be NULL. If |value| is NULL, the key is removed from the map. Is it useful to allow a NULL value to mean “remove the key?” I’d just as soon remove that and have this assert on a NULL argument, for better data validation. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:168: (entry.is_active() && strcmp(entry.key, key) == 0)) { The entry.is_active() after the or is unnecessary. It’s enough to say “if it’s not active or the key matches.” https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:168: (entry.is_active() && strcmp(entry.key, key) == 0)) { strNcmp for safety, again. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:170: if (!entry.is_active()) { Oh, oh no, no good. What if the map starts out like this 0 "robert": "sesek" 1 "mike": "pinkerton" 2 "mark": "mentovai" and then someone removes mike, so the map is 0 "robert": "sesek" 1 inactive 2 "mark": "mentovai" Then, someone tries adding mal to the map, which should replace me (although I should be unreplaceable!), but as written, the map winds up like this: 0 "robert": "sesek" 1 "mark": "larson" 2 "mark": "mentovai" Now there are two marks in the map. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:181: For the reason I mentioned above, when I write maps like this, I like to include a debug-mode integrity check (so you can write it as an assert) that scans the rest of the map after an insertion or removal to make sure that the key doesn’t appear again. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:195: memset(entries_[i].key, '\0', key_size); You don’t really need to zero the entire thing, it’s enough to just write key_size[0] = '\0' for performance, if you want. (Or to zero it out in debug mode?) https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:200: } In light of the above comments, you might want to provide a private internal function Entry* GetEntryForKey(const char* key) and implement each of GetValueForKey, SetKeyValue, and RemoveKey in terms of this new shared function. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:203: // for total_size bytes to dupliate the map. dupliCate https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:204: const Entry* map_entries() const { return entries_; } Aside from the test, there’s no reason this should be a public API, is there? https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... File src/common/simple_string_dictionary_unittest.cc (right): https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary_unittest.cc:36: NonAllocatingMap<10, 10, 10> map; You should (additionally or in this code’s place?) do these tests with the three template parameters set to different values, to weed out errors where a constant might be used in the wrong place in the implementation. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary_unittest.cc:61: EXPECT_EQ(strlen(map_entries[0].value), 0u); Add a test for the bug I found.
Sign in to reply to this message.
https://breakpad.appspot.com/568002/diff/1/src/client/mac/crash_generation/In... File src/client/mac/crash_generation/Inspector.h (right): https://breakpad.appspot.com/568002/diff/1/src/client/mac/crash_generation/In... src/client/mac/crash_generation/Inspector.h:68: KeyValueMessageData(const google_breakpad::SimpleStringDictionary::Entry &inEntry) { On 2013/04/23 21:45:54, Mark Mentovai wrote: > Mark this constructor explicit AND fix its 80-columness. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionary.h File src/common/simple_string_dictionary.h (right): https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:33: #include <cassert> On 2013/04/23 21:45:54, Mark Mentovai wrote: > <assert.h>, not <cassert>. And then take out the blank line between this and > <string.h> > > (If you were using a C++ header, the correct order would have been after > <string.h>.) Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:50: // and includes space for the NULL byte. This gives space for KeySize-1 and On 2013/04/23 21:45:54, Mark Mentovai wrote: > include, not includes > > NULL is a pointer value, not a byte value. NUL or '\0' is a byte value. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:51: // ValueSize-1 characters in an entry. On 2013/04/23 21:45:54, Mark Mentovai wrote: > Since the effective limits are KeySize-1 and ValueSize-1 but NumEntries (not > NumEntries-1) for the entries parameter, you might want to say so. Unless you > really do need room for a NULL entry, in which case, you should say that. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:53: class NonAllocatingMap { On 2013/04/23 21:45:54, Mark Mentovai wrote: > non_allocating_map.h? Ugh then I'd have to rename these files again. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:55: // The total amount of space the entry_map() is. On 2013/04/23 21:45:54, Mark Mentovai wrote: > What’s an entry_map()? Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:56: static const size_t total_size = (KeySize + ValueSize) * NumEntries; On 2013/04/23 21:45:54, Mark Mentovai wrote: > I’d put this after the three other static const size_ts, just because it’s kind > of more sensible that way when you’re reading from top to bottom. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:59: static const size_t key_size = KeySize; On 2013/04/23 21:45:54, Mark Mentovai wrote: > Do you want to name your constants like kKeySize, etc., so that it’s obvious > that they’re constants? Or do you hate Hungarians? I dunno. Making them Hungarian seems like it would be shared amongst all templated versions. This is what the STL does. Not that the STL is a paragon of good style. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:103: memset(&entries_, 0, sizeof(entries_)); On 2013/04/23 21:45:54, Mark Mentovai wrote: > Just writing an initializer list of entries_() is sufficient to zero out > entries_. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:106: NonAllocatingMap(const NonAllocatingMap& other) { On 2013/04/23 21:45:54, Mark Mentovai wrote: > If it doesn’t need to be copyable and assignable, I’d get rid of this > constructor and the assignment operator. It does. Other places within breakpad do this. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:111: assert(other.total_size == total_size); On 2013/04/23 21:45:54, Mark Mentovai wrote: > Elsewhere, following an assert, you have a check for the same condition the > assert is looking for, to prevent the code from doing anything too evil in > assertion-free release builds. You should consider doing that here too. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:111: assert(other.total_size == total_size); On 2013/04/23 21:45:54, Mark Mentovai wrote: > You want to check more than the total size, you want to make sure that the keys > and values are the same size too. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:137: if (entry.is_active() && strcmp(entry.key, key) == 0) { On 2013/04/23 21:45:54, Mark Mentovai wrote: > Use strNcmp for added safety since you have the maximum length handy in a > constant anyway. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:146: // must not be NULL. If |value| is NULL, the key is removed from the map. On 2013/04/23 21:45:54, Mark Mentovai wrote: > Is it useful to allow a NULL value to mean “remove the key?” I’d just as soon > remove that and have this assert on a NULL argument, for better data validation. I don't know what existing callers through SimpleStringDictionary are doing. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:168: (entry.is_active() && strcmp(entry.key, key) == 0)) { On 2013/04/23 21:45:54, Mark Mentovai wrote: > strNcmp for safety, again. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:168: (entry.is_active() && strcmp(entry.key, key) == 0)) { On 2013/04/23 21:45:54, Mark Mentovai wrote: > The entry.is_active() after the or is unnecessary. It’s enough to say “if it’s > not active or the key matches.” Not applicable anymore. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:170: if (!entry.is_active()) { On 2013/04/23 21:45:54, Mark Mentovai wrote: > Oh, oh no, no good. > > What if the map starts out like this > > 0 "robert": "sesek" > 1 "mike": "pinkerton" > 2 "mark": "mentovai" > > and then someone removes mike, so the map is > > 0 "robert": "sesek" > 1 inactive > 2 "mark": "mentovai" > > Then, someone tries adding mal to the map, which should replace me (although I > should be unreplaceable!), but as written, the map winds up like this: > > 0 "robert": "sesek" > 1 "mark": "larson" > 2 "mark": "mentovai" > > Now there are two marks in the map. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:181: On 2013/04/23 21:45:54, Mark Mentovai wrote: > For the reason I mentioned above, when I write maps like this, I like to include > a debug-mode integrity check (so you can write it as an assert) that scans the > rest of the map after an insertion or removal to make sure that the key doesn’t > appear again. I'd rather cover it with unit tests. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:195: memset(entries_[i].key, '\0', key_size); On 2013/04/23 21:45:54, Mark Mentovai wrote: > You don’t really need to zero the entire thing, it’s enough to just write > key_size[0] = '\0' for performance, if you want. (Or to zero it out in debug > mode?) Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:200: } On 2013/04/23 21:45:54, Mark Mentovai wrote: > In light of the above comments, you might want to provide a private internal > function > > Entry* GetEntryForKey(const char* key) > > and implement each of GetValueForKey, SetKeyValue, and RemoveKey in terms of > this new shared function. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:203: // for total_size bytes to dupliate the map. On 2013/04/23 21:45:54, Mark Mentovai wrote: > dupliCate Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:204: const Entry* map_entries() const { return entries_; } On 2013/04/23 21:45:54, Mark Mentovai wrote: > Aside from the test, there’s no reason this should be a public API, is there? Replaced with Serialized(). https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... File src/common/simple_string_dictionary_unittest.cc (right): https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary_unittest.cc:36: NonAllocatingMap<10, 10, 10> map; On 2013/04/23 21:45:54, Mark Mentovai wrote: > You should (additionally or in this code’s place?) do these tests with the three > template parameters set to different values, to weed out errors where a constant > might be used in the wrong place in the implementation. Done. https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary_unittest.cc:61: EXPECT_EQ(strlen(map_entries[0].value), 0u); On 2013/04/23 21:45:54, Mark Mentovai wrote: > Add a test for the bug I found. Done.
Sign in to reply to this message.
https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionary.h File src/common/simple_string_dictionary.h (right): https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:181: rsesek wrote: > On 2013/04/23 21:45:54, Mark Mentovai wrote: > > For the reason I mentioned above, when I write maps like this, I like to > include > > a debug-mode integrity check (so you can write it as an assert) that scans the > > rest of the map after an insertion or removal to make sure that the key > doesn’t > > appear again. > > I'd rather cover it with unit tests. But you don’t know that the tests cover all of the cases. The tests don’t know about/can’t fully exercise/can’t fully inspect the internals.
Sign in to reply to this message.
https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... File src/common/simple_string_dictionary.cc (right): https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.cc:39: union { Some compiler some day (today?) might warn about an anonymous type declaration that doesn’t also declare a variable. You can stave off warnings like that by writing something like “union U {”. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.cc:45: } // namespace google_breakpad Do you want to provide an explicit instantiation of all of SimpleStringDictionary here? template class NonAllocatingMap<256, 256, 64>; But it sucks to have to repeat the template arguments here, and it’s not that important, so maybe not. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... File src/common/simple_string_dictionary.h (right): https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:40: struct SerializedNonAllocatingMap; You should say something about this. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:58: static const size_t key_size = KeySize; I realize that the first two are just sizeofs applied to the fields in Entry… https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:60: static const size_t num_entries = NumEntries; …this is arraysize(entries_)… https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:63: static const size_t total_size = (KeySize + ValueSize) * NumEntries; …and this is just sizeof(entries_). It doesn’t make any difference for key_size, value_size, and num_entries, which are always just based on the template parameters, but for total_size it may become significant if alignment requirements ever cause padding to come into play. You might want to redefine total_size using sizeof instead of doing this arithmetic. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:113: assert(other.value_size == value_size); You have to provide an assertion about num_entries too (and consider it in the conditional on the following line). https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:155: // Stores |value| into |key|, replacing it if |key| already exists. |key| nit: replacing |key| if it is already present. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:167: // Key must not be empty string. Would an article have cost that much? https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... File src/common/simple_string_dictionary_unittest.cc (right): https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary_unittest.cc:246: EXPECT_STREQ("mal", map.GetValueForKey("mark")); This doesn’t actually test that "mark": "mal" isn’t shadowing "mark": "allays". You need to additionally: map.RemoveKey("mark"); EXPECT_EQ(1u, map.GetCount()); EXPECT_FALSE(map.GetValueForKey("mark")); This incomplete test is why I think it’s a good idea for the map itself to have its own integrity check. The test needs to exercise the code fully but the debug-mode integrity check can catch problems that a test wouldn’t be able to catch without being carefully crafted and without knowing or hypothesizing too much about implementation internals. In this example, the buggy map from patch set 1 with the shadowing problem would have asserted all on its own when you did map.SetKeyValue("mark", "mal") (as you wrote it here) without having to rely on the test doing map.RemoveKey("mark") subsequently. See how the two can work in concert to provide better coverage than either would be able to provide alone?
Sign in to reply to this message.
https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionary.h File src/common/simple_string_dictionary.h (right): https://breakpad.appspot.com/568002/diff/1/src/common/simple_string_dictionar... src/common/simple_string_dictionary.h:181: On 2013/04/24 02:11:47, Mark Mentovai wrote: > rsesek wrote: > > On 2013/04/23 21:45:54, Mark Mentovai wrote: > > > For the reason I mentioned above, when I write maps like this, I like to > > include > > > a debug-mode integrity check (so you can write it as an assert) that scans > the > > > rest of the map after an insertion or removal to make sure that the key > > doesn’t > > > appear again. > > > > I'd rather cover it with unit tests. > > But you don’t know that the tests cover all of the cases. The tests don’t know > about/can’t fully exercise/can’t fully inspect the internals. Done. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... File src/common/simple_string_dictionary.cc (right): https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.cc:39: union { On 2013/04/24 15:15:57, Mark Mentovai wrote: > Some compiler some day (today?) might warn about an anonymous type declaration > that doesn’t also declare a variable. You can stave off warnings like that by > writing something like “union U {”. Done. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.cc:45: } // namespace google_breakpad On 2013/04/24 15:15:57, Mark Mentovai wrote: > Do you want to provide an explicit instantiation of all of > SimpleStringDictionary here? > > template class NonAllocatingMap<256, 256, 64>; > > But it sucks to have to repeat the template arguments here, and it’s not that > important, so maybe not. Yeah, I thought about that but decided maybe not. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... File src/common/simple_string_dictionary.h (right): https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:40: struct SerializedNonAllocatingMap; On 2013/04/24 15:15:57, Mark Mentovai wrote: > You should say something about this. Done. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:58: static const size_t key_size = KeySize; On 2013/04/24 15:15:57, Mark Mentovai wrote: > I realize that the first two are just sizeofs applied to the fields in Entry… Fundamentally, how is that any different from this? https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:63: static const size_t total_size = (KeySize + ValueSize) * NumEntries; On 2013/04/24 15:15:57, Mark Mentovai wrote: > …and this is just sizeof(entries_). > > It doesn’t make any difference for key_size, value_size, and num_entries, which > are always just based on the template parameters, but for total_size it may > become significant if alignment requirements ever cause padding to come into > play. You might want to redefine total_size using sizeof instead of doing this > arithmetic. I just removed total_size since it's only ever used in serialization. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:113: assert(other.value_size == value_size); On 2013/04/24 15:15:57, Mark Mentovai wrote: > You have to provide an assertion about num_entries too (and consider it in the > conditional on the following line). Done. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:155: // Stores |value| into |key|, replacing it if |key| already exists. |key| On 2013/04/24 15:15:57, Mark Mentovai wrote: > nit: replacing |key| if it is already present. Done. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:167: // Key must not be empty string. On 2013/04/24 15:15:57, Mark Mentovai wrote: > Would an article have cost that much? Done. https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... File src/common/simple_string_dictionary_unittest.cc (right): https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary_unittest.cc:246: EXPECT_STREQ("mal", map.GetValueForKey("mark")); On 2013/04/24 15:15:57, Mark Mentovai wrote: > This doesn’t actually test that "mark": "mal" isn’t shadowing "mark": "allays". > You need to additionally: > > map.RemoveKey("mark"); > EXPECT_EQ(1u, map.GetCount()); > EXPECT_FALSE(map.GetValueForKey("mark")); > > This incomplete test is why I think it’s a good idea for the map itself to have > its own integrity check. The test needs to exercise the code fully but the > debug-mode integrity check can catch problems that a test wouldn’t be able to > catch without being carefully crafted and without knowing or hypothesizing too > much about implementation internals. > > In this example, the buggy map from patch set 1 with the shadowing problem would > have asserted all on its own when you did map.SetKeyValue("mark", "mal") (as you > wrote it here) without having to rely on the test doing map.RemoveKey("mark") > subsequently. See how the two can work in concert to provide better coverage > than either would be able to provide alone? Done.
Sign in to reply to this message.
LGTM https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... File src/common/simple_string_dictionary.h (right): https://breakpad.appspot.com/568002/diff/10/src/common/simple_string_dictiona... src/common/simple_string_dictionary.h:58: static const size_t key_size = KeySize; rsesek wrote: > On 2013/04/24 15:15:57, Mark Mentovai wrote: > > I realize that the first two are just sizeofs applied to the fields in Entry… > > Fundamentally, how is that any different from this? It’s not, that’s what I said when I picked on total_size. https://breakpad.appspot.com/568002/diff/14001/src/common/mac/BreakpadRelease... File src/common/mac/BreakpadRelease.xcconfig (right): https://breakpad.appspot.com/568002/diff/14001/src/common/mac/BreakpadRelease... src/common/mac/BreakpadRelease.xcconfig:34: GCC_PREPROCESSOR_DEFINITIONS = NDEBUG $(inherited) NDEBUG ? https://breakpad.appspot.com/568002/diff/14001/src/common/simple_string_dicti... File src/common/simple_string_dictionary.h (right): https://breakpad.appspot.com/568002/diff/14001/src/common/simple_string_dicti... src/common/simple_string_dictionary.h:200: if (strcmp(entries_[i].key, key) == 0) strNcmp as in GetConstEntryForKey. https://breakpad.appspot.com/568002/diff/14001/src/common/simple_string_dicti... src/common/simple_string_dictionary.h:223: } assert(GetEntryForKey(key) == NULL);
Sign in to reply to this message.
https://breakpad.appspot.com/568002/diff/14001/src/common/mac/BreakpadRelease... File src/common/mac/BreakpadRelease.xcconfig (right): https://breakpad.appspot.com/568002/diff/14001/src/common/mac/BreakpadRelease... src/common/mac/BreakpadRelease.xcconfig:34: GCC_PREPROCESSOR_DEFINITIONS = NDEBUG On 2013/04/24 17:10:33, Mark Mentovai wrote: > $(inherited) NDEBUG > ? Done. https://breakpad.appspot.com/568002/diff/14001/src/common/simple_string_dicti... File src/common/simple_string_dictionary.h (right): https://breakpad.appspot.com/568002/diff/14001/src/common/simple_string_dicti... src/common/simple_string_dictionary.h:200: if (strcmp(entries_[i].key, key) == 0) On 2013/04/24 17:10:33, Mark Mentovai wrote: > strNcmp as in GetConstEntryForKey. Done. https://breakpad.appspot.com/568002/diff/14001/src/common/simple_string_dicti... src/common/simple_string_dictionary.h:223: } On 2013/04/24 17:10:33, Mark Mentovai wrote: > assert(GetEntryForKey(key) == NULL); Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
