Closed Bug 1374629 Opened 2 years ago Closed 2 years ago

mozilla::Encoding constants (*_ENCODING) should be mozilla::NotNull

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: emk, Assigned: emk)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

I had to use WrapNotNull as a workaround. It is a bit ailly and adds a redundant run-time check.

The fix will require an upstream encoding_c change.
I didn't do this in the first place, because I wasn't sure if const Foo* and mozilla::NotNull<const Foo*> are guaranteed to have the same representation.

https://searchfox.org/mozilla-central/source/mfbt/NotNull.h#78 says "It has zero space overhead.", which implies they do have the same representation. njn, can that be relied upon?
Flags: needinfo?(n.nethercote)
JS already relies on that some wrapper types are binary compatible with the inner type.
> njn, can that be relied upon?

I've never thought about it in those exact terms, but "zero space overhead" and "has the same representation" are equivalent, AFAICT.

The class definition is trivial, as you'd expect, basically:

> template <typename T>
> class NotNull
> {
>   T mBasePtr;
> };

plus methods. It'd be bizarre to change it to anything else.

The static_assert in the 3rd patch looks good. Would it be worth adding another static_assert that the offset of mBasePtr is zero? Are there any other static_asserts required to guarantee the representation equivalence?
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #10)
> The static_assert in the 3rd patch looks good. Would it be worth adding
> another static_assert that the offset of mBasePtr is zero? Are there any
> other static_asserts required to guarantee the representation equivalence?

I tried to add
    static_assert(std::is_standard_layout<NotNull<T>>::value,
                  "NotNull must be a POD.");
to NotNull.h, , but Unified_cpp_xpcom_typelib_xpt0.cpp dislikes <type_traits> :(
 0:07.07 Unified_cpp_xpcom_typelib_xpt0.cpp
 0:07.07 D:/PROGRA~2/MICROS~1/VC\include\utility(94): error C2061: 構文エラー: 識別子 'enable_if_t'
 0:07.07 D:/PROGRA~2/MICROS~1/VC\include\utility(255): note: コンパイル対象のクラス テンプレート インスタンス化 'std::pair<_Ty1,_Ty2>' のリファレンスを確認してください
 0:07.07 D:/PROGRA~2/MICROS~1/VC\include\utility(95): error C2988: 認識できないテンプレートの宣言または定義です。
 0:07.07 D:/PROGRA~2/MICROS~1/VC\include\utility(95): error C2143: 構文エラー: ';' が '&&' の前にありません。
 0:07.07 D:/PROGRA~2/MICROS~1/VC\include\utility(95): error C2238: ';' の前に無効なトークンがあります。
 0:07.07 D:/PROGRA~2/MICROS~1/VC\include\utility(95): error C2059: 構文エラー: '&&'
 0:07.07 D:/PROGRA~2/MICROS~1/VC\include\utility(113): error C2238: ';' の前に無効なトークンがあります。
 0:07.07 D:/PROGRA~2/MICROS~1/VC\include\utility(119): error C2061: 構文エラー: 識別子 'enable_if_t'
 0:07.07 D:/PROGRA~2/MICROS~1/VC\include\utility(120): error C2988: 認識できないテンプレートの宣言または定義です。
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(120): error C2143: 構文エラー: ';' が '&&' の前にありません。
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(120): error C2238: ';' の前に無効なトークンがあります。
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(120): error C2059: 構文エラー: '&&'
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(155): error C2059: 構文エラー: '...'
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(156): error C2059: 構文エラー: '...'
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(158): error C2065: '_Types1': 定義されていない識別子です。
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(158): error C3544: '<unnamed-symbol>': パラメーター パックには型テンプ レート引数が必要です
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(159): error C2065: '_Types2': 定義されていない識別子です。
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(159): error C3544: '<unnamed-symbol>': パラメーター パックには型テンプ レート引数が必要です
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(159): error C2238: ';' の前に無効なトークンがあります。
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(165): error C2061: 構文エラー: 識別子 'enable_if_t'
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(166): error C2988: 認識できないテンプレートの宣言または定義です。
 0:07.08 D:/PROGRA~2/MICROS~1/VC\include\utility(166): error C2143: 構文エラー: ';' が '&&' の前にありません。
 0:07.09 D:/PROGRA~2/MICROS~1/VC\include\utility(166): error C2238: ';' の前に無効なトークンがあります。
 0:07.09 D:/PROGRA~2/MICROS~1/VC\include\utility(166): error C2059: 構文エラー: '&&'
 0:07.10 D:/PROGRA~2/MICROS~1/VC\include\utility(243): error C2238: ';' の前に無効なトークンがあります。
 0:07.10 D:/PROGRA~2/MICROS~1/VC\include\utility(249): error C2988: 認識できないテンプレートの宣言または定義です。
 0:07.10 D:/PROGRA~2/MICROS~1/VC\include\utility(249): error C2059: 構文エラー: 'if'
 0:07.10 D:/PROGRA~2/MICROS~1/VC\include\utility(250): error C2334: '{' の前に予期しないトークンがありました。関数の本体は無視されます
 0:07.10 D:/PROGRA~2/MICROS~1/VC\include\utility(255): error C2988: 認識できないテンプレートの宣言または定義です。
 0:07.10 D:/PROGRA~2/MICROS~1/VC\include\utility(255): error C2143: 構文エラー: ';' が '}' の前にありません。
 0:07.10 D:/PROGRA~2/MICROS~1/VC\include\utility(255): fatal error C1903: 直前のエラーを修復できません。コンパイルを中止します。
 0:07.10 f:/m/mozilla-unified/config/rules.mk:1010: recipe for target 'Unified_cpp_xpcom_typelib_xpt0.obj' failed
 0:07.10 mozmake.EXE[2]: *** [Unified_cpp_xpcom_typelib_xpt0.obj] Error 2

Adding 
static_assert(std::is_standard_layout<ENCODING_RS_CONST_ENCODING_PTR>::value,
              "NotNull must be a POD.");
to Encoding.h worked, but the more generic check would be better.
NotNull can be used to wrap smart pointers like RefPtr, so checking for POD isn't the right thing to do.
Depends on: 1374937
Comment on attachment 8879544 [details]
Bug 1374629 - Upstream patch

Obsoleted by bug 1374937.
Attachment #8879544 - Attachment is obsolete: true
Comment on attachment 8879545 [details]
Bug 1374629 - Add mozilla::NotNull to mozilla::Encoding constant declarations

https://reviewboard.mozilla.org/r/150844/#review156070

::: intl/Encoding.h:30
(Diff revision 3)
>  class Decoder;
>  class Encoder;
>  }; // namespace mozilla
>  
>  #define ENCODING_RS_ENCODING mozilla::Encoding
> +#define ENCODING_RS_CONST_ENCODING_PTR mozilla::NotNull<const mozilla::Encoding*>

Please use `ENCODING_RS_NON_NULL_CONST_ENCODING_PTR` as the macro name here and remove the `WrapNotNull`s from the calls to `encoding_for_name`, `encoding_output_encoding`, `decoder_encoding` and
`encoder_encoding`, too. r+ with those changes. Thanks.
Attachment #8879545 - Flags: review+
Comment on attachment 8879545 [details]
Bug 1374629 - Add mozilla::NotNull to mozilla::Encoding constant declarations

https://reviewboard.mozilla.org/r/150844/#review156070

> Please use `ENCODING_RS_NON_NULL_CONST_ENCODING_PTR` as the macro name here and remove the `WrapNotNull`s from the calls to `encoding_for_name`, `encoding_output_encoding`, `decoder_encoding` and
> `encoder_encoding`, too. r+ with those changes. Thanks.

The part about changing FFI calls no longer applies, obviously.
Attachment #8879545 - Attachment is obsolete: true
Attachment #8880039 - Flags: review?(nfroyd)
Comment on attachment 8880039 [details]
Bug 1374629 - Add mozilla::NotNull to mozilla::Encoding constant declarations.

https://reviewboard.mozilla.org/r/151370/#review156352

::: intl/Encoding.h:22
(Diff revision 1)
>  #include "mozilla/CheckedInt.h"
>  #include "mozilla/NotNull.h"
>  #include "mozilla/Span.h"
>  #include "mozilla/Tuple.h"
>  #include "nsString.h"
> +#include <type_traits>

r+ for things other than including `<type_traits>`. On that topic, deferring to :froydnj. If it's not OK to include that here, maybe the static asserts that require this header can  move to `TestEncoding.cpp` there the system header at least won't interfere with code that ships in the product.
Attachment #8880039 - Flags: review?(hsivonen) → review+
Comment on attachment 8880039 [details]
Bug 1374629 - Add mozilla::NotNull to mozilla::Encoding constant declarations.

https://reviewboard.mozilla.org/r/151370/#review156402

Henri's suggestion sounds good to me.  r=me with that change.

::: intl/Encoding.h:22
(Diff revision 1)
>  #include "mozilla/CheckedInt.h"
>  #include "mozilla/NotNull.h"
>  #include "mozilla/Span.h"
>  #include "mozilla/Tuple.h"
>  #include "nsString.h"
> +#include <type_traits>

When we have gone to replace `mozilla/TypeTraits.h` uses with `type_traits`, we have run into peculiar MSVC errors.  It looks like this patch compiles, and I suspect that we're not going to run into *too* many problems, because `Encoding.h` will not be included in a great number of places...but I would feel better if the `static_asserts` were moved to TestEncoding.cpp, as Henri mentioned.  We'll also waste slightly less compiling time including `type_traits` and repeatedly evaluating those `static_assert`s.
Attachment #8880039 - Flags: review?(nfroyd) → review+
Comment on attachment 8879593 [details]
Bug 1374629 - Ensure that mozilla::NotNull has zero space overhead.

https://reviewboard.mozilla.org/r/150914/#review156424

My only question about this is: does it prevent us from using NotNull<T> when T is forward-declared? UniquePtr<T> has that property and it's a bit annoying, because it can complicate the replacement of a T* in a header file with a UniquePtr<T>.
Attachment #8879593 - Flags: review?(n.nethercote) → review+
Comment on attachment 8880039 [details]
Bug 1374629 - Add mozilla::NotNull to mozilla::Encoding constant declarations.

https://reviewboard.mozilla.org/r/151370/#review156352

> r+ for things other than including `<type_traits>`. On that topic, deferring to :froydnj. If it's not OK to include that here, maybe the static asserts that require this header can  move to `TestEncoding.cpp` there the system header at least won't interfere with code that ships in the product.

Moved the static asserts to TestEncoding.cpp.
Comment on attachment 8879593 [details]
Bug 1374629 - Ensure that mozilla::NotNull has zero space overhead.

https://reviewboard.mozilla.org/r/150914/#review156424

It would not be a problem because T* will be replaced with NotNull<T*> unlike UniquePtr.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/19fbc34f239c
Add mozilla::NotNull to mozilla::Encoding constant declarations. r=froydnj,hsivonen
https://hg.mozilla.org/integration/autoland/rev/3411f1c818f6
Ensure that mozilla::NotNull has zero space overhead. r=njn
std::trivially_copyable support is very poor...
Flags: needinfo?(VYV03354)
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/864152fcb988
Add mozilla::NotNull to mozilla::Encoding constant declarations. r=froydnj,hsivonen
https://hg.mozilla.org/integration/autoland/rev/94a0efc7edc6
Ensure that mozilla::NotNull has zero space overhead. r=njn
https://hg.mozilla.org/mozilla-central/rev/864152fcb988
https://hg.mozilla.org/mozilla-central/rev/94a0efc7edc6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee: nobody → VYV03354
You need to log in before you can comment on or make changes to this bug.