Spinning this off of bug 922727. That bug is about visitCount being currently a signed int, which is silly because it's a count and counts can't be negative. Mak says in bug 922727 comment 1: > I think the original scope was to assert that they (both visitCount and > bookmarked) were always initialized before being read, though those asserts > don't exist as of now. > I think the best thing would be to make visitCount a uint (init to 0), > bookmarked a bool (init to false) and add an initialized field that we can > assert in debug mode. > Well, probably the cleanest thing would be to make both properties private, > expose a setter that sets both and initialized, and 2 getters, one for each, > that will assert if not initialized. Spinning this bug off to cover that ^.

Hi, Which solution is the best between case #1 and case #2 in this pastebin? http://pastebin.com/SGg38WEU Maybe there is already a templated class in mozilla such as IsSetProperty that i used in Case #2 Thanks,

We have a Maybe<T> template, which is effectively just a "T" and a bool that keeps track if the T is initialized or not. I think that's similar to "IsSetProperty<int> _property2;" in your pastebin. We don't want to use Maybe<> here, though. Here, the "initialized" flags are only supposed to be a sanity-check, and we only want them to exist in *debug* builds. And Maybe<> doesn't have any debug-specific stuff. So, I think we really do just need some bools that are #ifdef DEBUG.

(In reply to Daniel Holbert [:dholbert] from comment #2) > So, I think we really do just need some bools that are #ifdef DEBUG. Sorry, just re-read comment 0 -- sounds like Mak just wanted *one* "initialized" bool, with a single setter which sets both member-vars and the bool.

Hi, This should almost be what you asked for. I just have a question about thoses lines (begins at line 1808): > int32_t visitCount, bookmarked; > rv = stmt->GetInt32(5, &visitCount); > NS_ENSURE_SUCCESS(rv, rv); > rv = stmt->GetInt32(6, &bookmarked); visitCount type is supposed to be changed to a uint32_t and bookmarked to a bool, so which methods should i call to keep this behaviour? Thanks, PS: i will also upload a separate indent patch (after this one is completed) for class PlaceHashKey as the indentation in the class definition doesn't seems correct in m-c

Comment on attachment 8482950 [details] [diff] [review] refactor_PlaceHashKey_to_avoid_static_cast.patch Review of attachment 8482950 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/History.cpp @@ +211,5 @@ > { > MOZ_ASSERT(false, "Do not call me!"); > } > > + void setProperties(uint visitCountValue, bool bookmarkedValue) Use "uint32_t", not "uint". (This applies to getVisitCount(), too.) Also, the function name should be capitalized, per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods (This applies to getVisitCount and getBookmarked, too.) The arguments should probably be named "aVisitCount" and "aIsBookmarked", too, per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes @@ +225,5 @@ > + { > +#ifdef DEBUG > + MOZ_ASSERT(isSetProperty, "PlaceHashKey::visitCount not set"); > +#endif > + return (visitCount); Drop the unnecessary parenthesis around (visitCount) here, in getVisitCount(). Same for getBookmarked's return statement. @@ +228,5 @@ > +#endif > + return (visitCount); > + } > + > + bool getBookmarked() const Perhaps this should be named "isBookmarked"? Either way is probably fine, though. @@ +245,5 @@ > + // Whether this place is bookmarked. > + bool bookmarked; > + // Whether previous attributes are set. > +#ifdef DEBUG > + bool isSetProperty; I don't understand what "isSetProperty" means. (the word "property" seems out of place) Maybe call this "isInitialized"? (or "mIsInitialized", except that the other member-vars don't use the "m" prefix, so maybe it'd be out of place until we convert them.) @@ +1844,1 @@ > rv = stmt->GetInt32(5, &visitCount); Per your comment here, this does need to be changed. Probably best to match what we do above it for "transition" -- use an int32_t local var, and then static_case when we call SetProperties().

Sorry, that last part was missing the context. The context was: >- int32_t visitCount, bookmarked; >+ uint32_t visitCount; >+ bool bookmarked; >+ //This needs to be changed > rv = stmt->GetInt32(5, &visitCount);

previous patch was marked Part1 instead of Part2

i forgot to remove a useless comment in the previous patch (thanks dholbert :) )

Tbpl is green: https://tbpl.mozilla.org/?tree=Try&rev=c780ddb9557d

Comment on attachment 8483008 [details] [diff] [review] refactor_PlaceHashKey_to_avoid_static_cast.patch Part1/3 Review of attachment 8483008 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/History.cpp @@ +220,5 @@ > + isInitialized = true; > +#endif > + } > + > + uint32_t GetVisitCount() const just VisitCount() will be fine, or you should also have GetIsBookmarked. let's keep the less verbose versions, so VisitCount() and IsBookmarked(). @@ +244,5 @@ > + uint32_t visitCount; > + // Whether this place is bookmarked. > + bool bookmarked; > + // Whether previous attributes are set. > +#ifdef DEBUG please move the comment inside the ifdef

Comment on attachment 8483001 [details] [diff] [review] fix_indentation_toolkit_History.patch Part2/3 Review of attachment 8483001 [details] [diff] [review]: ----------------------------------------------------------------- thank you for this cleanup

Fixing previous version with Mak's comments (keeping the review+)

Hi, part 2 and part3 failed to apply like: renamed 1049812 -> rename_PlaceHashKey_members_with_m_prefix.patch applying rename_PlaceHashKey_members_with_m_prefix.patch patching file toolkit/components/places/History.cpp Hunk #1 FAILED at 182 Hunk #2 FAILED at 1599 Hunk #3 FAILED at 1690 3 out of 4 hunks FAILED -- saving rejects to file toolkit/components/places/History.cpp.rej patch failed, unable to continue (try -v) and renamed 1049812 -> fix_indentation_toolkit_History.patch applying fix_indentation_toolkit_History.patch patching file toolkit/components/places/History.cpp Hunk #2 FAILED at 231 1 out of 2 hunks FAILED -- saving rejects to file toolkit/components/places/History.cpp.rej could you take a look and maybe rebase on a current tree, thanks!

Hi, You have to import fix_indentation_toolkit_History.patch before rename_PlaceHashKey_members_with_m_prefix.patch this is why they are named Part 2/3 and Part 3/3 :)

I get conflicts when applying part 1/3 on a clean trunk, too. (probably from patch conflicts with bug 1060974, which landed in the last few days and touched History.cpp)

I discussed with Tomcat this morning and there is a patch in inbound that is preventing from applying correctly. I will wait until it lands and fix them all. Thanks for the feedback

Thoses are the same patches just with the last changes that where applied in bug 1060974 as Dholbert said. I kept the review+ as nothing changed in my patches. tbpl is still green: https://tbpl.mozilla.org/?tree=Try&rev=15096625607c

Comment on attachment 8484495 [details] [diff] [review] refactor_PlaceHashKey_to_avoid_static_cast.patch Part1/3 >diff --git a/toolkit/components/places/History.cpp b/toolkit/components/places/History.cpp >- , bookmarked(-1) >+ , visitCount(0) >+ , bookmarked(false) visits; >+private: > // Visit count for this place. > int32_t visitCount; > // Whether this place is bookmarked. > int32_t bookmarked; Looks like you're no longer doing the int32_t --> uint32_t type conversion (for visitCount) and int32_t --> bool (for 'bookmarked') conversions here. Those conversions were in the previous version of this patch (attachment 8483693 [details] [diff] [review]), but are missing in this version.

Yup unfortunatly you're totally right... i didn't watch this correctly when modifying my patches... Thanks for your eagle eyes that prevented thoses wrong patches to get landed. :)

Daniel can you take a last look at it to confirm that this time it's okay?

Yup, looks good now. (I just compared it side-by-side with the original (reviewed but bitrotted) patches, switching tabs back and forth to look for differences.) Adding 'checkin-needed' back.

https://hg.mozilla.org/integration/fx-team/rev/27ea646472f9 https://hg.mozilla.org/integration/fx-team/rev/d38ec8e9cfd6 https://hg.mozilla.org/integration/fx-team/rev/750dac4dd853

https://hg.mozilla.org/mozilla-central/rev/27ea646472f9 https://hg.mozilla.org/mozilla-central/rev/d38ec8e9cfd6 https://hg.mozilla.org/mozilla-central/rev/750dac4dd853