Bug 184164 – Make WTF::String poisonable

• Component: Web Template Framework
• Status: RESOLVED WONTFIX

[JF Bastien]

.

[JF Bastien]

<rdar://problem/39020677>

[JF Bastien]

Created attachment 336819 [details]
patch

[EWS Watchlist]

Attachment 336819 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Forward.h:159:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WTF/wtf/text/AtomicString.h:221:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:544:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:138:  The parameter name "str" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:176:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:190:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:193:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:194:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:197:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:198:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:214:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:256:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:257:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:259:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:260:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:428:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:429:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:449:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:450:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:457:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:473:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:544:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/ChangeLog:94:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WTF/wtf/text/WTFString.cpp:443:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/WTFString.cpp:976:  The parameter name "impl" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/text/WTFString.cpp:977:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 27 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 336819 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336819&action=review

Initial thoughts:
This can still be split into many smaller easier-to-review-and-land patches.
1) assume COMPILER_SUPPORTS(CXX_REFERENCE_QUALIFIED_FUNCTIONS) is always true
2) remove WTF_EXPORT_STRING_API
3) introduce WTF_LAZY_INSTANTIATE
4) remove declarations of WTF::String
5) change codePointCompareLessThan and WTF::characters
6) fix const and nullptr and clean up some things
7) introduce a template parameter to String

I'm also not sure I like having BasicString and PoisonableString.  Could PoisonableString just be String?

> Source/WebCore/ChangeLog:35
> +        Neither base has any data members.

I don't think we need to duplicate this whole message 6 times.

> Source/WebCore/ChangeLog:94
> +        No new tests (OOPS!).

This should be removed.

[JF Bastien]

Sounds good, I'll start with the split you suggest!

[JF Bastien]

I added he first 3 patches you suggested as blocking this one. I'll continue with the others later, meatspace calls for now.

[Filip Pizlo]

Comment on attachment 336819 [details]
patch

1) As we discussed yesterday in person, we shouldn't land this because of our new plan.
2) This approach is far too complicated for the small value it introduces.

[JF Bastien]

> I'm also not sure I like having BasicString and PoisonableString.  Could
> PoisonableString just be String?

Forgot to reply to this part: it could be, but then String is templated and has a default template parameter. It then means that in quite a few places you need to spell it out as String<> just to get the default. One of my earlier approaches did that and it was pretty ugly.

Now I have:
  using String = PoisonableString<>;

Another option is to have:
  class String : public PoisonableString<> {
  public:
    // All the constructors are re-implemented here to forward to the base class.
  };

I didn't try it out but it seemed more pain that just the using type alias.