Closed Bug 1319016 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Details

Attachments

(2 files)

We can use constexpr in this case and make IsVariant class inherit that.
Attachment #8812712 - Flags: review?(nfroyd)
Attachment #8812713 - Flags: review?(nfroyd)
attach the try result to make sure this can be compiled for all platform

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9366942356de07345dd132928d7b06e99fb2bdc1
Comment on attachment 8812713 [details]
Bug 1319016 - Part2 - Make IsVariant class use IntegralConstant as true false type.

https://reviewboard.mozilla.org/r/94362/#review94574
Attachment #8812713 - Flags: review?(nfroyd) → review+
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?
Attachment #8812712 - Flags: review?(nfroyd) → review+
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.
Keywords: checkin-needed
Please resolve the pending issues in MozReview so that this can be autolanded.
Keywords: checkin-needed
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.
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;
Pushed by nfroyd@mozilla.com:
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
(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;
}
https://hg.mozilla.org/mozilla-central/rev/74be7a5aa2ed
https://hg.mozilla.org/mozilla-central/rev/db2ce61de2ce
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
#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].
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.
You need to log in before you can comment on or make changes to this bug.