Improve the warning about attempting to overwrite prefs with a pref of the wrong type

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

The warning is currently in the form "Trying to overwrite value of default pref %s with the wrong type". This could be improved by:

 a) making it clear this attempt will be IGNORED
 b) mentioning the types involved
Posted patch patchSplinter Review
Attachment #8921847 - Flags: review?(n.nethercote)
Comment on attachment 8921847 [details] [diff] [review]
patch

Review of attachment 8921847 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nits below addressed.

Also, I've reformatted libpref/ with clang-format, so please do that (`./mach clang-format` will do the current patch) if you haven't already.

::: modules/libpref/Preferences.cpp
@@ +164,5 @@
>    Bool = 3,
>  };
>  
> +#ifdef DEBUG
> +const char* PrefTypeToString(PrefType aType)

Return type on its own line (clang-format should do that for you).

@@ +166,5 @@
>  
> +#ifdef DEBUG
> +const char* PrefTypeToString(PrefType aType)
> +{
> +  static const char* kInvalidStr = "INVALID";

No need for the named constants, just do `return "INVALID"`, etc., below.

@@ +181,5 @@
> +    return kIntStr;
> +  case PrefType::Bool:
> +    return kBoolStr;
> +  default:
> +    MOZ_ASSERT_UNREACHABLE("Unhandled enum value");

The comment for MOZ_ASSERT_UNREACHABLE says

> Unconditional assert in debug builds for (assumed) unreachable code paths
> that have a safe return without crashing in release builds.

The code currently doesn't have a safe return in release builds. You could either add `return "???";` after the MOZ_ASSERT_UNREACHABLE, or you could change it to MOZ_CRASH.
Attachment #8921847 - Flags: review?(n.nethercote) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49451ebb746c
Improve the warning about attempting to overwrite prefs with a pref of the wrong type. r=njn
https://hg.mozilla.org/mozilla-central/rev/49451ebb746c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.