Closed Bug 1237151 Opened 4 years ago Closed 4 years ago

Enable -Wignored-qualifiers for C++ code

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

We use -Wignored-qualifiers for C code. We should do likewise for C++ code, to make C and C++ build commands more similar.
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.
We talked on IRC about the risk of this breaking the ABI; hopefully not.
Attachment #8704906 - Flags: review?(cpearce)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
I think it's ok to modify all these files, but not 100% certain. Especially
media/webrtc/signaling/src/sdp/Sdp.h.
Attachment #8704907 - Flags: review?(rjesup)
The XXX comments are due to the issue described in comment 1. I'm happy to file
a follow-up to fix them up.
Attachment #8704909 - Flags: review?(nfroyd)
It's already on for C code.
Attachment #8704911 - Flags: review?(mshal)
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
Attachment #8704907 - Flags: review?(rjesup)
Attachment #8704907 - Flags: review?(docfaraday)
Attachment #8704907 - Flags: review+
Attachment #8704906 - Flags: review?(cpearce) → review+
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.
Attachment #8704907 - Flags: review?(docfaraday) → review+
Attachment #8704911 - Flags: review?(mshal) → review+
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.
Attachment #8704909 - Flags: review?(nfroyd) → review+
> ::: 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.
Blocks: 1238105
I filed bug 1238105 for fixing the SafeElementAt() callsites.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.