1766886 - Refactor ComputedTimingFunction to use mozilla::Variant

Assignee

Description

•

3 years ago

Currently ComputedTimingFunction has two mutually exclusive objects stored together with an enum tag. With bug 1764126, there will be another element added - looks like a good time to refactor and use mozilla::Variant.

David Shin[:dshin]

Assignee

Updated

•

3 years ago

Blocks: 1764126

Updated

•

3 years ago

Assignee: nobody → dshin

Status: NEW → ASSIGNED

David Shin[:dshin]

Assignee

Comment 1

•

3 years ago

Attached file Bug 1766886 - Use `mozilla::variant` in `ComputedTimingFunction` to contain the function implementation. r=boris — Details

Also minor refactorings:

Less Init() calls
Remove unused Compare functions

Pulsebot

Comment 2

•

3 years ago

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a7fae9e0df25 Use `mozilla::variant` in `ComputedTimingFunction` to contain the function implementation. r=boris

Iulian Moraru

Comment 3

•

3 years ago

Backed out for causing build bustages on TimingParams.h.

Push with failures

Failure log

Backout link

[task 2022-05-03T20:07:02.098Z] 20:07:02     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/accessible/html'
[task 2022-05-03T20:07:02.100Z] 20:07:02     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/gcc/bin/g++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -std=gnu++17 -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/c++/7.5.0 -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/x86_64-linux-gnu/c++/7.5.0 -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/x86_64-linux-gnu -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include -o Unified_cpp_accessible_html0.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/accessible/html -I/builds/worker/workspace/obj-build/accessible/html -I/builds/worker/checkouts/gecko/accessible/base -I/builds/worker/checkouts/gecko/accessible/generic -I/builds/worker/checkouts/gecko/accessible/xpcom -I/builds/worker/checkouts/gecko/layout/forms -I/builds/worker/checkouts/gecko/layout/generic -I/builds/worker/checkouts/gecko/layout/tables -I/builds/worker/checkouts/gecko/layout/xul -I/builds/worker/checkouts/gecko/accessible/atk -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wno-invalid-offsetof -Wduplicated-cond -Wimplicit-fallthrough -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wformat -Wformat-overflow=2 -Wno-psabi -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -gdwarf-4 -freorder-blocks -Os -fno-omit-frame-pointer -funwind-tables -Werror -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/Unified_cpp_accessible_html0.o.pp   Unified_cpp_accessible_html0.cpp
[task 2022-05-03T20:07:02.111Z] 20:07:02     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/layers/AnimationStorageData.h:15:0,
[task 2022-05-03T20:07:02.112Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/mozilla/layers/AnimationInfo.h:15,
[task 2022-05-03T20:07:02.113Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/mozilla/layers/WebRenderUserData.h:13,
[task 2022-05-03T20:07:02.113Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/nsDisplayListInvalidation.h:11,
[task 2022-05-03T20:07:02.114Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/nsDisplayList.h:51,
[task 2022-05-03T20:07:02.115Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/layout/generic/nsImageFrame.h:17,
[task 2022-05-03T20:07:02.115Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/html/HTMLImageMapAccessible.cpp:14,
[task 2022-05-03T20:07:02.115Z] 20:07:02     INFO -                   from Unified_cpp_accessible_html0.cpp:29:
[task 2022-05-03T20:07:02.116Z] 20:07:02    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/TimingParams.h:32:13: error: explicitly defaulted function 'constexpr mozilla::TimingParams::TimingParams()' cannot be declared as constexpr because the implicit declaration is not constexpr:
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -     constexpr TimingParams() = default;
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -               ^~~~~~~~~~~~
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/nsTSubstring.h:18:0,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/nsAString.h:22,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/nsString.h:16,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/nsAtom.h:16,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/mozilla/CachedInheritingStyles.h:10,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/mozilla/ComputedStyle.h:13,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/generic/LocalAccessible.h:9,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/atk/AccessibleWrap.h:11,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/generic/HyperTextAccessible.h:9,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/atk/HyperTextAccessibleWrap.h:10,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/html/HTMLCanvasAccessible.h:9,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/html/HTMLCanvasAccessible.cpp:6,
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -                   from Unified_cpp_accessible_html0.cpp:2:
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:372:33: note: defaulted constructor calls non-constexpr 'mozilla::Maybe<T>::Maybe() [with T = mozilla::ComputedTimingFunction]'
[task 2022-05-03T20:07:02.116Z] 20:07:02     INFO -     MOZ_ALLOW_TEMPORARY constexpr Maybe() = default;
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                                   ^~~~~
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:372:33: note: 'mozilla::Maybe<T>::Maybe() [with T = mozilla::ComputedTimingFunction]' is not usable as a constexpr function because:
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:247:3: note: defaulted constructor calls non-constexpr 'mozilla::detail::MaybeStorage<T, false>::MaybeStorage() [with T = mozilla::ComputedTimingFunction]'
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -     MaybeStorage() = default;
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -     ^~~~~~~~~~~~
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:247:3: note: 'mozilla::detail::MaybeStorage<T, false>::MaybeStorage() [with T = mozilla::ComputedTimingFunction]' is not usable as a constexpr function because:
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:20:0,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/nsTSubstring.h:18,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/nsAString.h:22,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/nsString.h:16,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/nsAtom.h:16,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/mozilla/CachedInheritingStyles.h:10,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/workspace/obj-build/dist/include/mozilla/ComputedStyle.h:13,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/generic/LocalAccessible.h:9,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/atk/AccessibleWrap.h:11,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/generic/HyperTextAccessible.h:9,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/atk/HyperTextAccessibleWrap.h:10,
[task 2022-05-03T20:07:02.117Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/html/HTMLCanvasAccessible.h:9,
[task 2022-05-03T20:07:02.118Z] 20:07:02     INFO -                   from /builds/worker/checkouts/gecko/accessible/html/HTMLCanvasAccessible.cpp:6,
[task 2022-05-03T20:07:02.118Z] 20:07:02     INFO -                   from Unified_cpp_accessible_html0.cpp:2:
[task 2022-05-03T20:07:02.118Z] 20:07:02     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/MaybeStorageBase.h:49:3: note: defaulted constructor calls non-constexpr 'mozilla::detail::MaybeStorageBase<T, false>::MaybeStorageBase() [with T = mozilla::ComputedTimingFunction]'
[task 2022-05-03T20:07:02.118Z] 20:07:02     INFO -     MaybeStorageBase() = default;
[task 2022-05-03T20:07:02.118Z] 20:07:02     INFO -     ^~~~~~~~~~~~~~~~
[task 2022-05-03T20:07:02.118Z] 20:07:02     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/MaybeStorageBase.h:49:3: note: 'mozilla::detail::MaybeStorageBase<T, false>::MaybeStorageBase() [with T = mozilla::ComputedTimingFunction]' is not usable as a constexpr function because:
[task 2022-05-03T20:07:02.119Z] 20:07:02     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/MaybeStorageBase.h:34:5: note: defaulted constructor calls non-constexpr 'mozilla::detail::MaybeStorageBase<T, false>::Union::Union() [with T = mozilla::ComputedTimingFunction]'
[task 2022-05-03T20:07:02.119Z] 20:07:02     INFO -       Union() {}
[task 2022-05-03T20:07:02.119Z] 20:07:02     INFO -       ^~~~~
[task 2022-05-03T20:07:02.119Z] 20:07:02    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:660: Unified_cpp_accessible_html0.o] Error 1
[task 2022-05-03T20:07:02.119Z] 20:07:02     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/accessible/html'
[task 2022-05-03T20:07:02.119Z] 20:07:02    ERROR -  gmake[3]: *** [/builds/worker/checkouts/gecko/config/recurse.mk:72: accessible/html/target-objects] Error 2
[task 2022-05-03T20:07:02.119Z] 20:07:02     INFO -  gmake[3]: *** Waiting for unfinished jobs....
[task 2022-05-03T20:07:02.177Z] 20:07:02     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/config/external/ffi'

Flags: needinfo?(dshin)

David Shin[:dshin]

Assignee

Comment 4

•

3 years ago

Interesting, failing on gcc but not clang.
Working on it.

Flags: needinfo?(dshin)

David Shin[:dshin]

Assignee

Comment 5

•

3 years ago

Attached file Min reproducible compilation issue — Details

Min repro case:

dshin@david-moz:~/mozilla-unified$ g++ repro.cpp 
repro.cpp:24:13: error: explicitly defaulted function ‘constexpr ContainsMaybe::ContainsMaybe()’ cannot be declared as ‘constexpr’ because the implicit declaration is not ‘constexpr’:
   24 |   constexpr ContainsMaybe() = default;
      |             ^~~~~~~~~~~~~
repro.cpp:17:13: note: defaulted constructor calls non-‘constexpr’ ‘Maybe<T>::Maybe() [with T = std::vector<int>]’
   17 |   constexpr Maybe() = default;
      |             ^~~~~
repro.cpp:17:13: note: ‘Maybe<T>::Maybe() [with T = std::vector<int>]’ is not usable as a ‘constexpr’ function because:
repro.cpp:7:3: note: defaulted constructor calls non-‘constexpr’ ‘MaybeStorage<T>::MaybeStorage() [with T = std::vector<int>]’
    7 |   MaybeStorage() = default;
      |   ^~~~~~~~~~~~
repro.cpp:7:3: note: ‘MaybeStorage<T>::MaybeStorage() [with T = std::vector<int>]’ is not usable as a ‘constexpr’ function because:
In file included from /usr/include/c++/9/vector:67,
                 from repro.cpp:1:
/usr/include/c++/9/bits/stl_vector.h:484:7: note: defaulted constructor calls non-‘constexpr’ ‘std::vector<_Tp, _Alloc>::vector() [with _Tp = int; _Alloc = std::allocator<int>]’
  484 |       vector() = default;
      |       ^~~~~~
/usr/include/c++/9/bits/stl_vector.h:484:7: note: ‘std::vector<_Tp, _Alloc>::vector() [with _Tp = int; _Alloc = std::allocator<int>]’ is not usable as a ‘constexpr’ function because:
/usr/include/c++/9/bits/stl_vector.h:285:7: note: defaulted constructor calls non-‘constexpr’ ‘std::_Vector_base<_Tp, _Alloc>::_Vector_base() [with _Tp = int; _Alloc = std::allocator<int>]’
  285 |       _Vector_base() = default;
      |       ^~~~~~~~~~~~
/usr/include/c++/9/bits/stl_vector.h:285:7: note: ‘std::_Vector_base<_Tp, _Alloc>::_Vector_base() [with _Tp = int; _Alloc = std::allocator<int>]’ is not usable as a ‘constexpr’ function because:
/usr/include/c++/9/bits/stl_vector.h:128:2: note: defaulted constructor calls non-‘constexpr’ ‘std::_Vector_base<_Tp, _Alloc>::_Vector_impl::_Vector_impl() [with _Tp = int; _Alloc = std::allocator<int>]’
  128 |  _Vector_impl() _GLIBCXX_NOEXCEPT_IF(
      |  ^~~~~~~~~~~~

but on clang:

dshin@david-moz:~/mozilla-unified$ ~/.mozbuild/clang/bin/clang++ repro.cpp 
dshin@david-moz:~/mozilla-unified$

Replacing the ctor with the one under // Succeeds w/ both makes it compile on both.
... Not 100% on why, though - [:botond], is there anything I should be concerned about this approach?

Flags: needinfo?(botond)

Botond Ballo [:botond]

Comment 6

•

3 years ago

I think this comes down to the following paragraph in the C++ standard (dcl.fct.def.default/3):

An explicitly-defaulted function that is not defined as deleted may be declared constexpr or consteval only if it is constexpr-compatible [...]

Basically, the rules for explicitly-defaulted functions are fairly strict, and only allow the constexpr specifier to be present if the generated function does in fact turn out to be constexpr.

This means, I believe, that gcc is technically correct to reject the code.

Note, the paragraph goes on to say (emphasis mine):

A function explicitly defaulted on its first declaration is [...] implicitly constexpr if it is constexpr-compatible.

This means that another fix that should be portable across compilers would be to remove the explicit constexpr specifier from the defaulted declaration:

ContainsMaybe() = default;

and the constructor will nevertheless be implicitly constexpr if possible.

Finally, worth noting that this restrictive behaviour of the explicit constexpr specifier for defaulted functions is being removed in this standards proposal, which has recently been accepted for C++23.

Conclusion: it's fine to go with the workaround you found, or alternatively the workaround I suggest above.

Flags: needinfo?(botond)

Pulsebot

Comment 7

•

3 years ago

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f93b73ab6c1c Use `mozilla::variant` in `ComputedTimingFunction` to contain the function implementation. r=boris

Iulian Moraru

Comment 8

•

3 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/f93b73ab6c1c

Status: ASSIGNED → RESOLVED

Closed: 3 years ago

status-firefox102: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 102 Branch

Bug 1766886 - Use `mozilla::variant` in `ComputedTimingFunction` to contain the function implementation. r=boris 3 years ago David Shin[:dshin] 48 bytes, text/x-phabricator-request		Details \| Review
Min reproducible compilation issue 3 years ago David Shin[:dshin] 599 bytes, text/x-c++src		Details

Bugzilla

Refactor ComputedTimingFunction to use mozilla::Variant

Categories

(Core :: Layout, enhancement)

Tracking

()

People

(Reporter: dshin, Assigned: dshin)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(2 files)

Description

Updated

Updated

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Attachment

General

Description

File Name

Content Type