Patch coming.
Aren’t these the same thing?
(In reply to Saam Barati from comment #1) > Aren’t these the same thing? Yes, just changing to use standard C++.
Created attachment 318616 [details] proposed patch.
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.
(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.
Comment on attachment 318616 [details] proposed patch. r=me
(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.
Comment on attachment 318616 [details] proposed patch. Thanks for the review.
Comment on attachment 318616 [details] proposed patch. Clearing flags on attachment: 318616 Committed r221002: <http://trac.webkit.org/changeset/221002>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34006794>
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.
(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.