1237151 - Enable -Wignored-qualifiers for C++ code

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nicholas Nethercote [inactive]]

We use -Wignored-qualifiers for C code. We should do likewise for C++ code, to make C and C++ build commands more similar.

Comment 1

[Nicholas Nethercote [inactive]]

I've identified one interesting thing here, which comes up in nsTArray.h. Consider this signature:

>  const E* SafeElementAt(index_type aIndex) const;

This function returns a pointer to an E which is const, i.e. cannot be modified through the pointer. That's fine.

This code looks equivalent:

>  typedef E*       elem_type;
>  const elem_type SafeElementAt(index_type aIndex) const;

but it's actually not; the |const| applies to the pointer itself, not to the memory being pointed to. So GCC complains about this when -Wignored-qualifiers is used.

The right way to do this is as follows:

>  typedef const E* const_elem_type;
>  const_elem_type SafeElementAt(index_type aIndex) const;

It's an easy mistake to make, so it's another argument in favour of enabling -Wignored-qualifiers.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove ignored qualifiers in dom/media/gmp/

We talked on IRC about the risk of this breaking the ABI; hopefully not.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove ignored qualifiers in WebRTC-relate code

I think it's ok to modify all these files, but not 100% certain. Especially
media/webrtc/signaling/src/sdp/Sdp.h.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove ignored qualifiers in all remaining code

The XXX comments are due to the issue described in comment 1. I'm happy to file
a follow-up to fix them up.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Turn on -Wignored-qualifiers for C++ code

It's already on for C code.

Comment 6

[Nicholas Nethercote [inactive]]

Try server looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d39ec93ec50

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8704907 [details] [diff] [review]
(part 2) - Remove ignored qualifiers in WebRTC-relate code

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

r? to bwc for Sdp.h

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8704907 [details] [diff] [review]
(part 2) - Remove ignored qualifiers in WebRTC-relate code

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

The change to Sdp.h looks fine to me.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8704909 [details] [diff] [review]
(part 3) - Remove ignored qualifiers in all remaining code

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

Looks like this warning reduces clutter and catches real errors. \o/

::: embedding/components/printingui/unixshared/nsPrintingPromptService.cpp
@@ +42,5 @@
>      nsresult Init() {
>        return CallCreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID, &mBlock);
>      }
>      nsIDialogParamBlock * operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN { return mBlock; }
> +    operator nsIDialogParamBlock * () const { return mBlock; }

Whoops!

::: gfx/layers/Layers.cpp
@@ +945,1 @@
>  Layer::GetLocalOpacity()

I think this method can be const...though maybe we'd need a const version of AsLayerComposite(), and that might get messy.  Follow-up?

::: ipc/ipdl/ipdl/lower.py
@@ +576,5 @@
>          t.ptr = 0
>          t.ptrconstptr = 1
>          return t
>      t.const = 1
> +    t.ptr = 1

This is so we don't generate |const T* const| when we don't need to, correct?

::: netwerk/cache2/CacheObserver.h
@@ -26,5 @@
>    static nsresult Shutdown();
>    static CacheObserver* Self() { return sSelf; }
>  
>    // Access to preferences
> -  static bool const UseNewCache();

Are you sure these weren't supposed to be const methods instead?

::: toolkit/crashreporter/google-breakpad/src/common/unique_string.cc
@@ +87,5 @@
>    string key(str, n);
>    return sUSU->FindOrCopy(key);
>  }
>  
> +char Index(const UniqueString* us, int ix)

I think all these toolkit/crashreport/google-breakpad/ are third-party sources, so we should just turn the warning off in a moz.build.

::: toolkit/mozapps/update/updater/automounter_gonk.h
@@ +26,5 @@
>  public:
>    GonkAutoMounter();
>    ~GonkAutoMounter();
>  
> +  MountAccess GetAccess()

Probably a const method?

::: tools/profiler/lul/LulCommon.cpp
@@ +76,5 @@
>    ~UniqueString() { free(reinterpret_cast<void*>(const_cast<char*>(str_))); }
>    const char* str_;
>  };
>  
> +const char* FromUniqueString(const UniqueString* ustr)

Probably a const method?

::: xpcom/glue/nsTArray.h
@@ +270,5 @@
>  
> +  // XXX: Probably should return const_elem_type, but callsites must be fixed.
> +  // Also, the use of const_elem_type for nsTArray<xpcGCCallback> in
> +  // xpcprivate.h causes build failures on Windows because xpcGCCallback is a
> +  // function pointer and MSVC doesn't like qualifying it with |const|.

I guess we could fix this by polyfilling std::add_const and then using it to define const_elem_type.

Please file a followup bug for fixing callsites.

@@ +292,5 @@
>      return static_cast<Derived*>(this)->SafeElementAt(aIndex, nullptr);
>    }
>  
> +  // XXX: Probably should return const_elem_type, but callsites must be fixed.
> +  elem_type SafeElementAt(index_type aIndex) const

Please file a followup bug.

@@ +332,5 @@
>      return nullptr;
>    }
>  
> +  // XXX: Probably should return const_elem_type, but callsites must be fixed.
> +  elem_type SafeElementAt(index_type aIndex) const

Likewise.

Comment 10

[Nicholas Nethercote [inactive]]

> ::: gfx/layers/Layers.cpp
> @@ +945,1 @@
> >  Layer::GetLocalOpacity()
> 
> I think this method can be const...though maybe we'd need a const version of
> AsLayerComposite(), and that might get messy.  Follow-up?
 
I tried, and it did get messy. I don't think it's worth a follow-up.


> ::: ipc/ipdl/ipdl/lower.py
> @@ +576,5 @@
> >          t.ptr = 0
> >          t.ptrconstptr = 1
> >          return t
> >      t.const = 1
> > +    t.ptr = 1
> 
> This is so we don't generate |const T* const| when we don't need to, correct?

Yes.

 
> > +  MountAccess GetAccess()
> 
> Probably a const method?

Yes.


> ::: netwerk/cache2/CacheObserver.h
> @@ -26,5 @@
> >    static nsresult Shutdown();
> >    static CacheObserver* Self() { return sSelf; }
> >  
> >    // Access to preferences
> > -  static bool const UseNewCache();
> 
> Are you sure these weren't supposed to be const methods instead?

They're all static, so const doesn't apply.


> ::: toolkit/crashreporter/google-breakpad/src/common/unique_string.cc
> @@ +87,5 @@
> >    string key(str, n);
> >    return sUSU->FindOrCopy(key);
> >  }
> >  
> > +char Index(const UniqueString* us, int ix)
> 
> I think all these toolkit/crashreport/google-breakpad/ are third-party
> sources, so we should just turn the warning off in a moz.build.

Ok.


> ::: tools/profiler/lul/LulCommon.cpp
> @@ +76,5 @@
> >    ~UniqueString() { free(reinterpret_cast<void*>(const_cast<char*>(str_))); }
> >    const char* str_;
> >  };
> >  
> > +const char* FromUniqueString(const UniqueString* ustr)
> 
> Probably a const method?

It's a standalone function, not a member function.


> Please file a followup bug for fixing callsites.

Will do.

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb6c701de879e57ba4655d9e0bd48f9011da6f5
Bug 1237151 (part 1) - Remove ignored qualifiers in dom/media/gmp/. r=cpearce.

https://hg.mozilla.org/integration/mozilla-inbound/rev/729e181c80897c948337afaa7f49484b29f88ea8
Bug 1237151 (part 2) - Remove ignored qualifiers in WebRTC-relate code. r=jesup.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2b9869729288b7e236410a45b45e80ba4c7ad6
Bug 1237151 (part 3) - Remove ignored qualifiers in all remaining code. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/01908255691fb869756ce0e16caea539e2a87bd6
Bug 1237151 (part 4) - Turn on -Wignored-qualifiers for C++ code. r=mshal.

Comment 12

[Nicholas Nethercote [inactive]]

Try looked good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37999326c0cb

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cb6c701de87
https://hg.mozilla.org/mozilla-central/rev/729e181c8089
https://hg.mozilla.org/mozilla-central/rev/fd2b98697292
https://hg.mozilla.org/mozilla-central/rev/01908255691f

Comment 14

[Nicholas Nethercote [inactive]]

I filed bug 1238105 for fixing the SafeElementAt() callsites.