1319016 - Make IntegralConstant::value use constexpr and make IsVariant class use IntegralConstant as true false type

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[James Cheng[:JamesCheng]]

We can use constexpr in this case and make IsVariant class inherit that.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1319016 - Part1 - Make IntegralConstant::value use constexpr.

Review commit: https://reviewboard.mozilla.org/r/94360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94360/

Comment 2

[James Cheng[:JamesCheng]]

Attachment: Bug 1319016 - Part2 - Make IsVariant class use IntegralConstant as true false type.

Review commit: https://reviewboard.mozilla.org/r/94362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94362/

Comment 3

[James Cheng[:JamesCheng]]

attach the try result to make sure this can be compiled for all platform

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9366942356de07345dd132928d7b06e99fb2bdc1

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8812713 [details]
Bug 1319016 - Part2 - Make IsVariant class use IntegralConstant as true false type.

https://reviewboard.mozilla.org/r/94362/#review94574

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8812712 [details]
Bug 1319016 - Part1 - Make IntegralConstant::value use constexpr.

https://reviewboard.mozilla.org/r/94360/#review94576

r=me, but I would like to understand this change better.

::: mfbt/TypeTraits.h:49
(Diff revision 1)
>   * because <type_traits> exposes it as well.
>   */
>  template<typename T, T Value>
>  struct IntegralConstant
>  {
> -  static const T value = Value;
> +  static constexpr T value = Value;

Why is the change to `constexpr` necessary here?  AFAICT, you're doing this so that `IsVariant` in the next patch can be used in `static_assert`...I think?  It's not at all clear to me.

But we use various things inheriting from `IntegralConstant` in `static_assert` and the like already, so I don't see what practical difference this change makes.  Is this just bringing `IntegralConstant` more in line with `std::integral_constant`, or is it done for some other reason?

Comment 6

[James Cheng[:JamesCheng]]

Thank you.

As you mentioned, we already used those traits in static_assert which means it has been compile-time already...

Intuitively, just want IntegralConstant to be more in line with std version.

Make it more compile-time-like semantic.

Initialization "static const integral = value"  inside class did not mean to define this symbol. But few people know about it, they might think static const may take some extra space to store. 

That's why I want to make the change.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please resolve the pending issues in MozReview so that this can be autolanded.

Comment 8

[Jeff Walden [:Waldo]]

I somewhat doubt people think static const <integral_type> takes up space -- or at least they're worrying too hard if that's their concern.  It's also worth noting that you can take the address of constexprs, so it's not really correct to say they don't take up space either.  But hey, no reason not to constexpr this stuff up.

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

For me, "more in line with std version" is the best argument for this change. (Which I too was initially dubious about!)

From N3337 20.9.3 [meta.help]:
>    template <class T, T v>
>    struct integral_constant {
>      static constexpr T value = v;

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74be7a5aa2ed
Part1 - Make IntegralConstant::value use constexpr. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/db2ce61de2ce
Part2 - Make IsVariant class use IntegralConstant as true false type. r=froydnj

Comment 11

[James Cheng[:JamesCheng]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> I somewhat doubt people think static const <integral_type> takes up space --
> or at least they're worrying too hard if that's their concern.  It's also
> worth noting that you can take the address of constexprs, so it's not really
> correct to say they don't take up space either.  But hey, no reason not to

Cannot take address of static constexprs and static const

Do you mean this[1]? This only allowed in VC but I think it is not the right behavior.
> constexpr this stuff up.


[1]
#include <iostream>
using namespace std;
struct Foo
{
	static const int a= 10;
	static constexpr int b = 10;
};
int main() {
	cout<<&Foo::a<<endl; //link error
	cout<<&Foo::b<<endl; //link error
	return 0;
}

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74be7a5aa2ed
https://hg.mozilla.org/mozilla-central/rev/db2ce61de2ce

Comment 13

[Jeff Walden [:Waldo]]

#include <type_traits>
#include <stdio.h>

int main()
{
  using IC = std::integral_constant<bool, true>;

  const bool* addr = &IC::value;
  printf("%p\n", (void*)addr);
};

compiles for me with recent Clang.  I'm pretty sure it's valid C++.  There's no no-constexpr restriction in C++11 [expr.unary.op].

Comment 14

[James Cheng[:JamesCheng]]

notice that for g++ implementation , below is the code snippet

#include <iostream>
using namespace std;
template<typename _Tp, _Tp __v>
struct integral_constant2
{
  static constexpr _Tp                  value = __v;
  typedef _Tp                           value_type;
  typedef integral_constant<_Tp, __v>   type;
};
  
// template<typename _Tp, _Tp __v>
// constexpr _Tp integral_constant2<_Tp, __v>::value;

int main() {
	cout<<&integral_constant2<bool, true>::value;
	return 0;
}

If we comment out the definition outside the class, we cannot take the address anymore(link error).

This is what I want to express.

Once we do not define the static const/constexpr outside of class.

We cannot take the address of that since there is not really a definition even we assign a value inside the class.