Bug 175762 – Change probe code to use static_assert instead of COMPILE_ASSERT.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Saam Barati]

Aren’t these the same thing?

[Mark Lam]

(In reply to Saam Barati from comment #1)
> Aren’t these the same thing?

Yes, just changing to use standard C++.

[Mark Lam]

Created attachment 318616 [details]
proposed patch.

[Saam Barati]

The nice thing about COMPILE_ASSERT is that it automatically fills in the string message. You’ve just doubled the amount of characters switching to this basically to just repeat what COMPILE_ASSERT would’ve done for you. That said, I still end up using static_assert myself mostly because I’m not sure what our official style recommendation is here for WebKit. FWIW, I prefer COMPILE_ASSERT here.

[Mark Lam]

(In reply to Saam Barati from comment #4)
> The nice thing about COMPILE_ASSERT is that it automatically fills in the
> string message. You’ve just doubled the amount of characters switching to
> this basically to just repeat what COMPILE_ASSERT would’ve done for you.
> That said, I still end up using static_assert myself mostly because I’m not
> sure what our official style recommendation is here for WebKit. FWIW, I
> prefer COMPILE_ASSERT here.

FWIW, I'm not married to this change.  I don't know what the official policy is on COMPILE_ASSERT, but as a project, we've always migrated to using standard C++ constructs once they become available.  static_assert is available in C++11, and as of sept 2013, we required C++11 (see https://webkit.org/blog/3172/webkit-and-cxx11/).  In fact, that WebKit blog entry specifically calls out the benefits of using static_assert.

As for the number of characters doubling, I think you're just using a hyperbole here to express the idea.  I didn't literally double the number of characters, though I did add a few.  As I see it, the advantage of static_assert is that it allows the error message to be more expressive.  For example, I can say "stuff[x] should be 5" instead of stuff_x_should_be_5 (which is how I would need to express it for COMPILE_ASSERT).  This makes the expression more human readable.  So, I think the extra characters are justified here.

In terms of memory usage, since this is a static_assert, I think it only takes up extra space in the source code and not in the product.  It should generate no artifacts in the product at all.

[JF Bastien]

Comment on attachment 318616 [details]
proposed patch.

r=me

[Saam Barati]

(In reply to Mark Lam from comment #5)
> (In reply to Saam Barati from comment #4)
> > The nice thing about COMPILE_ASSERT is that it automatically fills in the
> > string message. You’ve just doubled the amount of characters switching to
> > this basically to just repeat what COMPILE_ASSERT would’ve done for you.
> > That said, I still end up using static_assert myself mostly because I’m not
> > sure what our official style recommendation is here for WebKit. FWIW, I
> > prefer COMPILE_ASSERT here.
> 
> FWIW, I'm not married to this change.  I don't know what the official policy
> is on COMPILE_ASSERT, but as a project, we've always migrated to using
> standard C++ constructs once they become available.  static_assert is
> available in C++11, and as of sept 2013, we required C++11 (see
> https://webkit.org/blog/3172/webkit-and-cxx11/).  In fact, that WebKit blog
> entry specifically calls out the benefits of using static_assert.
> 
> As for the number of characters doubling, I think you're just using a
> hyperbole here to express the idea.  I didn't literally double the number of
> characters, though I did add a few.  As I see it, the advantage of
> static_assert is that it allows the error message to be more expressive. 
> For example, I can say "stuff[x] should be 5" instead of stuff_x_should_be_5
> (which is how I would need to express it for COMPILE_ASSERT).  This makes
> the expression more human readable.  So, I think the extra characters are
> justified here.
> 
> In terms of memory usage, since this is a static_assert, I think it only
> takes up extra space in the source code and not in the product.  It should
> generate no artifacts in the product at all.

I thought you were originally using a variant of COMPILE_ASSERT that did not take a second operand. Such a variant does not exist, I thought it had. If you were actually using such a variant, you really would’ve doubled the number of characters. Clearly I didn’t read your code that closely :)

I had some vague memory of seeing us use such a COMPILE_ASSERT somewhere but I must be misremembering. I’m now in favor of this change.

[Mark Lam]

Comment on attachment 318616 [details]
proposed patch.

Thanks for the review.

[WebKit Commit Bot]

Comment on attachment 318616 [details]
proposed patch.

Clearing flags on attachment: 318616

Committed r221002: <http://trac.webkit.org/changeset/221002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34006794>

[Saam Barati]

When I made my comment, I was thinking about LLIntData's STATIC_ASSERT macro:
#define STATIC_ASSERT(cond) static_assert(cond, "LLInt assumes " #cond)

I kind of like that more than static_assert when the condition is a good enough description. I'm not suggesting to do that here, but that's what I was confusing COMPILE_ASSERT for.

[Mark Lam]

(In reply to Saam Barati from comment #12)
> When I made my comment, I was thinking about LLIntData's STATIC_ASSERT macro:
> #define STATIC_ASSERT(cond) static_assert(cond, "LLInt assumes " #cond)
> 
> I kind of like that more than static_assert when the condition is a good
> enough description. I'm not suggesting to do that here, but that's what I
> was confusing COMPILE_ASSERT for.

With C++17, we should be able to specify static_assert without a message, not to mention nothing stops us from passing a “” message today, though I’m not sure if there’s a style issue with that.