Issue 29722755: Issue 6378 - [emscripten] Make DependentString constexpr
|
Created:
March 14, 2018, 3:50 p.m. by René Jeschke Modified:
March 19, 2018, 1:49 p.m. CC:
Oleksandr Visibility:
Public. |
DescriptionIssue 6378 - [emscripten] Make DependentString constexpr
Patch Set 1 #Patch Set 2 : Added missing 'constexpr' to data() #
Total comments: 3
Patch Set 3 : Added test case. #
Total comments: 4
Patch Set 4 : More tests, made operator[] and default ctor constexpr. #
Total comments: 1
Patch Set 5 : Made _str constexpr. #
Total comments: 19
Patch Set 6 : More constexprs, test cases. #
Total comments: 2
Patch Set 7 : Seperated test cases. #
Total comments: 7
Patch Set 8 : Even more constexpr and tests. #
Total comments: 2
Patch Set 9 : Removed accidential whitespace. #Patch Set 10 : Added missing '*'. #MessagesTotal messages: 27
Could you add a couple of tests merely demonstrating that it works in compile time? https://codereview.adblockplus.org/29722755/diff/29722758/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29722758/compiled/String.h#n... compiled/String.h:227: explicit DependentString() what about this one too?
Will add a test case for this. https://codereview.adblockplus.org/29722755/diff/29722758/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29722758/compiled/String.h#n... compiled/String.h:227: explicit DependentString() On 2018/03/14 16:22:53, sergei wrote: > what about this one too? Is there a reason why we would want invalid nullptr constexpr strings? We can't change them, so they would be absolutely useless. If there's a use-case for those, I'll make this one constexpr, too.
Added a simple test case.
https://codereview.adblockplus.org/29722755/diff/29722760/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29722760/test/compiled/Strin... test/compiled/String.cpp:29: EXPECT_FALSE(s.is_writable()); I wonder whether it should be static_assert too.
https://codereview.adblockplus.org/29722755/diff/29722760/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29722760/test/compiled/Strin... test/compiled/String.cpp:29: EXPECT_FALSE(s.is_writable()); On 2018/03/14 16:57:44, sergei wrote: > I wonder whether it should be static_assert too. Yeah, was thinking about this, too. I wasn't sure if I can have a test case without any 'tests'. And: if 's.length()' works without errors, the other functions should, too, as the static assert makes sure, that we really have a constexpr value. I can, add more static_asserts, one for each 'constexpr' method, if wanted.
https://codereview.adblockplus.org/29722755/diff/29722758/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29722758/compiled/String.h#n... compiled/String.h:227: explicit DependentString() On 2018/03/14 16:27:07, René Jeschke wrote: > On 2018/03/14 16:22:53, sergei wrote: > > what about this one too? > > Is there a reason why we would want invalid nullptr constexpr strings? We can't > change them, so they would be absolutely useless. > > If there's a use-case for those, I'll make this one constexpr, too. It's difficult to say right now, and it's also up to a compiler as I guess, e.g. it's used in containers (Map, Set), so maybe it would be safer to have it constexpr too, so it does not accidentally prevent e.g. some global constant Map from being constexpr.
https://codereview.adblockplus.org/29722755/diff/29722760/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29722760/test/compiled/Strin... test/compiled/String.cpp:29: EXPECT_FALSE(s.is_writable()); On 2018/03/14 17:01:00, René Jeschke wrote: > On 2018/03/14 16:57:44, sergei wrote: > > I wonder whether it should be static_assert too. > > Yeah, was thinking about this, too. I wasn't sure if I can have a test case > without any 'tests'. I think it's fine, JIC technically it's possible. > And: if 's.length()' works without errors, the other > functions should, too, as the static assert makes sure, that we really have a > constexpr value. > > I can, add more static_asserts, one for each 'constexpr' method, if wanted. That would be very good too.
More tests, made operator[] and default ctor constexpr. https://codereview.adblockplus.org/29722755/diff/29722760/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29722760/test/compiled/Strin... test/compiled/String.cpp:29: EXPECT_FALSE(s.is_writable()); On 2018/03/14 17:03:09, sergei wrote: > On 2018/03/14 17:01:00, René Jeschke wrote: > > On 2018/03/14 16:57:44, sergei wrote: > > > I wonder whether it should be static_assert too. > > > > Yeah, was thinking about this, too. I wasn't sure if I can have a test case > > without any 'tests'. > I think it's fine, JIC technically it's possible. > > And: if 's.length()' works without errors, the other > > functions should, too, as the static assert makes sure, that we really have a > > constexpr value. > > > > I can, add more static_asserts, one for each 'constexpr' method, if wanted. > > That would be very good too. Done.
constexpr-s are awesome LGTM.
LGTM
Could you please rebase it? https://codereview.adblockplus.org/29722755/diff/29722763/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29722763/compiled/String.h#n... compiled/String.h:301: inline DependentString operator "" _str(const String::value_type* str, Can it be constexpr too? http://en.cppreference.com/w/cpp/language/user_literal
On 2018/03/15 15:58:37, sergei wrote: > Could you please rebase it? > > https://codereview.adblockplus.org/29722755/diff/29722763/compiled/String.h > File compiled/String.h (right): > > https://codereview.adblockplus.org/29722755/diff/29722763/compiled/String.h#n... > compiled/String.h:301: inline DependentString operator "" _str(const > String::value_type* str, > Can it be constexpr too? > http://en.cppreference.com/w/cpp/language/user_literal Done.
https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:84: ~String() = default; We're not defining copy or move constructors, so there doesn't seem to be any need to define a default destructor. Omitting this would make the destructor public, but there's no downside to that. https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:93: constexpr size_type length() const constexpr implies const, so the trailing declaration can be dropped here and in several places below. https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:130: bool equals(const String& other) const I'd make this private, since its behavior is exposed through operator==. https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:138: bool operator==(const String& other) const This can be constexpr, no? https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:320: inline constexpr DependentString operator "" _str(const String::value_type* str, constexpr on functions implies inline, so it could be dropped. https://codereview.adblockplus.org/29722755/diff/29723638/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29723638/test/compiled/Strin... test/compiled/String.cpp:24: TEST(TestString, checkConstexprCorrectness) Since all the static_assert statements don't generate runtime code, there's no particular need to have this function be in a test suite. An ordinary void function would work just as well. I'd also separate these tests into three functions, one for each of the constexpr constructors. Also, the s3 expression is (implicitly) combining a test for one of these constructors with one for _str(), which deserves to be tested separately in a fourth function.
https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:84: ~String() = default; On 2018/03/15 17:05:00, Eric wrote: > We're not defining copy or move constructors, so there doesn't seem to be any > need to define a default destructor. Omitting this would make the destructor > public, but there's no downside to that. What was the reason to make the dtor protected in the beginning? https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:93: constexpr size_type length() const On 2018/03/15 17:05:00, Eric wrote: > constexpr implies const, so the trailing declaration can be dropped here and in > several places below. No, it does not. 'constexpr' on an object declaration implies 'const', not on a function declaration. https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:130: bool equals(const String& other) const On 2018/03/15 17:05:00, Eric wrote: > I'd make this private, since its behavior is exposed through operator==. 'equals' is already used in 'compiled/filter/RegExpFilter.cpp', so I won't make it private. https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:138: bool operator==(const String& other) const On 2018/03/15 17:05:01, Eric wrote: > This can be constexpr, no? Yep, 'equals' and 'operator==' can both be 'constexpr'. Done. https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:320: inline constexpr DependentString operator "" _str(const String::value_type* str, On 2018/03/15 17:05:01, Eric wrote: > constexpr on functions implies inline, so it could be dropped. Right. Done. https://codereview.adblockplus.org/29722755/diff/29723638/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29723638/test/compiled/Strin... test/compiled/String.cpp:24: TEST(TestString, checkConstexprCorrectness) On 2018/03/15 17:05:01, Eric wrote: > Since all the static_assert statements don't generate runtime code, there's no > particular need to have this function be in a test suite. An ordinary void > function would work just as well. > > I'd also separate these tests into three functions, one for each of the > constexpr constructors. Also, the s3 expression is (implicitly) combining a test > for one of these constructors with one for _str(), which deserves to be tested > separately in a fourth function. Is it really worth it to clutter the test code with _four_ functions not containing any code but static_asserts only? Also, testing ctors will result in functions only having one variable definition ... which then would need to include a dummy test to prevent 'unused variable' warnings. The 's3' test case is fixed now, though, and tests for 'equals' and '==' are included. Also I've made the function a non-test one.
https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:84: ~String() = default; On 2018/03/15 17:41:23, René Jeschke wrote: > What was the reason to make the dtor protected in the beginning? Don't know. If there's a good reason, it deserves a one-line comment. Sergei? Hubert? https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:93: constexpr size_type length() const On 2018/03/15 17:41:23, René Jeschke wrote: > On 2018/03/15 17:05:00, Eric wrote: > > constexpr implies const, so the trailing declaration can be dropped here and > in > > several places below. > > No, it does not. 'constexpr' on an object declaration implies 'const', not on a > function declaration. Bah. Misread the spec. Never mind. https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:130: bool equals(const String& other) const On 2018/03/15 17:41:22, René Jeschke wrote: > On 2018/03/15 17:05:00, Eric wrote: > > I'd make this private, since its behavior is exposed through operator==. > > 'equals' is already used in 'compiled/filter/RegExpFilter.cpp', so I won't make > it private. OK. It would be better to change it there, but that's getting outside the scope of the current change set. https://codereview.adblockplus.org/29722755/diff/29723638/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29723638/test/compiled/Strin... test/compiled/String.cpp:24: TEST(TestString, checkConstexprCorrectness) I should preface these comments by saying that I always seem to test code far more exhaustively than most. Lots of people think it's overkill; I don't. On 2018/03/15 17:41:23, René Jeschke wrote: > On 2018/03/15 17:05:01, Eric wrote:> Is it really worth it to clutter the test code with _four_ functions not > containing any code but static_asserts only? In my experience, it would be. One main goal of unit testing to be able to isolate faults as quickly as possible. Splitting up the tests by the particular function/constructor that the test arises in speeds up that process. I wouldn't call it clutter, either. The names of these functions provide documentation about which specific thing is being tested. The real value of this isn't now, when the code is still fresh, but later, when it's being rewritten. It's then that all the testing overhead really comes into its own. It's cheap to write the simple tests about assumptions; it's much lengthier to reverse-engineer them later, particularly when it's someone else who's having their first encounter. > Also, testing ctors will result in functions only having one variable definition > ... which then would need to include a dummy test to prevent 'unused variable' > warnings. Are you getting such warnings now? Take out the static_assert statements, which only run at compile time, and all you've got left are constructors and initialization expressions. Depends on the compiler, but simple instantiation of a non-POD class doesn't generally give that warning (presumably for possible constructor side-effects). The first test I usually write on a class is just a default-constructed instance (if appropriate) with no other statements. (The test is that the constructor doesn't throw.) I don't see such unused variable messages. https://codereview.adblockplus.org/29722755/diff/29723645/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29723645/test/compiled/Strin... test/compiled/String.cpp:42: static_assert(s2.is_invalid(), "String should be invalid"); Does calling empty(), is_writable(), etc. on invalid strings return a placeholder result or do they throw? Do we care about this difference in behavior?
https://codereview.adblockplus.org/29722755/diff/29723638/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29723638/test/compiled/Strin... test/compiled/String.cpp:24: TEST(TestString, checkConstexprCorrectness) On 2018/03/15 18:33:54, Eric wrote: > I should preface these comments by saying that I always seem to test code far > more exhaustively than most. Lots of people think it's overkill; I don't. > > On 2018/03/15 17:41:23, René Jeschke wrote: > > On 2018/03/15 17:05:01, Eric wrote:> Is it really worth it to clutter the test > code with _four_ functions not > > containing any code but static_asserts only? > > In my experience, it would be. One main goal of unit testing to be able to > isolate faults as quickly as possible. Splitting up the tests by the particular > function/constructor that the test arises in speeds up that process. I wouldn't > call it clutter, either. The names of these functions provide documentation > about which specific thing is being tested. > > The real value of this isn't now, when the code is still fresh, but later, when > it's being rewritten. It's then that all the testing overhead really comes into > its own. It's cheap to write the simple tests about assumptions; it's much > lengthier to reverse-engineer them later, particularly when it's someone else > who's having their first encounter. > > > Also, testing ctors will result in functions only having one variable > definition > > ... which then would need to include a dummy test to prevent 'unused variable' > > warnings. > > Are you getting such warnings now? Take out the static_assert statements, which > only run at compile time, and all you've got left are constructors and > initialization expressions. > > Depends on the compiler, but simple instantiation of a non-POD class doesn't > generally give that warning (presumably for possible constructor side-effects). > The first test I usually write on a class is just a default-constructed instance > (if appropriate) with no other statements. (The test is that the constructor > doesn't throw.) I don't see such unused variable messages. > Okay, fine with me. I separated the tests. The only warning I got was for 's = u"..."_str', so I used this one for function/operator tests.
LGTM
https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:84: ~String() = default; On 2018/03/15 18:33:54, Eric wrote: > On 2018/03/15 17:41:23, René Jeschke wrote: > > What was the reason to make the dtor protected in the beginning? > > Don't know. If there's a good reason, it deserves a one-line comment. Sergei? > Hubert? Such class hierarchy caused questions from the beginning but we agreed to go with it for the sake of convenience and performance. However it's not supposed that an instance of `String` can live without either `DependentString` or `OwnedString`, so having of the protected destructor ensures that no one can forget to call the destructor of `OwnedString`. https://codereview.adblockplus.org/29722755/diff/29723638/compiled/String.h#n... compiled/String.h:130: bool equals(const String& other) const On 2018/03/15 18:33:54, Eric wrote: > On 2018/03/15 17:41:22, René Jeschke wrote: > > On 2018/03/15 17:05:00, Eric wrote: > > > I'd make this private, since its behavior is exposed through operator==. > > > > 'equals' is already used in 'compiled/filter/RegExpFilter.cpp', so I won't > make > > it private. > > OK. It would be better to change it there, but that's getting outside the scope > of the current change set. Yes, it's outside the scope of this issue and there is no reason to make it private. However we could perhaps get rid of `equals`, though it seems it does not really harm. Please, create an issue for it if you think it is worth discussing and let's continue there. https://codereview.adblockplus.org/29722755/diff/29723645/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29723645/test/compiled/Strin... test/compiled/String.cpp:42: static_assert(s2.is_invalid(), "String should be invalid"); On 2018/03/15 18:33:54, Eric wrote: > Does calling empty(), is_writable(), etc. on invalid strings return a > placeholder result or do they throw? > Do we care about this difference in behavior? Could you please create an issue for that? Since now we have native tests one can cover such cases in tests. Additionally it would be better to add more checks of arguments, e.g. DependentString(nullptr, 10) looks like an invalid string or there is perhaps even something wrong. Basically nothing is supposed to throw here when it runs in JS, however if someone wants, it's possible to achieve it. For invalid string `empty` returns true and `is_writable` returns false. https://codereview.adblockplus.org/29722755/diff/29723662/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29723662/compiled/String.h#n... compiled/String.h:143: bool operator!=(const String& other) const In this case this should be constexpr too. https://codereview.adblockplus.org/29722755/diff/29723662/compiled/String.h#n... compiled/String.h:148: size_type find(value_type c, size_type pos = 0) const As well as this. https://codereview.adblockplus.org/29722755/diff/29723662/compiled/String.h#n... compiled/String.h:156: size_type find(const String& str, size_type pos = 0) const and this and so on. https://codereview.adblockplus.org/29722755/diff/29723662/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29723662/test/compiled/Strin... test/compiled/String.cpp:31: static constexpr DependentString s; What is the reason to have `static` variables?
https://codereview.adblockplus.org/29722755/diff/29723662/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29722755/diff/29723662/compiled/String.h#n... compiled/String.h:143: bool operator!=(const String& other) const On 2018/03/16 13:43:27, sergei wrote: > In this case this should be constexpr too. Yep, will make 'constexpr' what can be 'constexpr' and add tests for those. https://codereview.adblockplus.org/29722755/diff/29723662/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29723662/test/compiled/Strin... test/compiled/String.cpp:31: static constexpr DependentString s; On 2018/03/16 13:43:27, sergei wrote: > What is the reason to have `static` variables? It's an old habit. As I am using 'const' a lot, I want to make a difference between 'is-not-gonna-change' and 'does-never-ever-change'. E.g. 'const len = strlen(chars);' and 'static const len = 10;'. I can remove the static modifier it it bothers someone.
https://codereview.adblockplus.org/29722755/diff/29723662/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29723662/test/compiled/Strin... test/compiled/String.cpp:31: static constexpr DependentString s; On 2018/03/16 17:00:47, René Jeschke wrote: > On 2018/03/16 13:43:27, sergei wrote: > > What is the reason to have `static` variables? > > It's an old habit. As I am using 'const' a lot, I want to make a difference > between 'is-not-gonna-change' and 'does-never-ever-change'. E.g. 'const len = > strlen(chars);' and 'static const len = 10;'. > > I can remove the static modifier it it bothers someone. Yes, please.
So, addressed last issues, new (and hopefully final) patch set is up.
LGTM
https://codereview.adblockplus.org/29722755/diff/29727575/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29727575/test/compiled/Strin... test/compiled/String.cpp:13: Could you please revert the change.
https://codereview.adblockplus.org/29722755/diff/29727575/test/compiled/Strin... File test/compiled/String.cpp (right): https://codereview.adblockplus.org/29722755/diff/29727575/test/compiled/Strin... test/compiled/String.cpp:13: On 2018/03/19 13:33:04, sergei wrote: > Could you please revert the change. Done.
LGTM
LGTM 
|
