r61052 MediaWiki - Code Review archive

Borked UTF-8

Fixed on r61052

Coding style:

• There should be space after #-symbol.

I'm ignoring the brace on new line issue since the file is using the other style for now.

And don't we use "elseif" rather than "else if"?? Reminds me of the dash polic on enwiki though...

Nikerabbit, noted.

Happy-melon: That section uses "else if"s. Parser.php seems quite confused: $ grep -c "elseif" Parser.php 35

$ grep -c "else if" Parser.php 36

Lol. It does have a throwaway mention in Coding conventions, though. Someone should do a find-and-replace throughout...

The coding style does indeed fit in nicely with r4549. However, it doesn't fit in with the rest of the file or with modern MW coding conventions, or even with basic rules of good taste. So I would like to see it cleaned up. I'm talking variable names, indenting, braces, comments, etc.

} else if ($arr[$i-1] != "") {

Suggest !==. Reasons on Manual:Coding conventions.

The r4549 algorithm in general has a serious problem with worst-case memory usage, due to the use of preg_split(). For a megabyte of double quotes, I measure a memory usage of 130MB. I'd like to see it replaced by string traversal with strpos/strspn, with the "preliminary" phase converted into a preprocessor or incorporated into the second phase.

I'm not sure what the point of outputting </b><b> is. Tidy will probably remove it. Why don't you just leave it out?

Otherwise seems OK.

I had left just to keep it clearer, since that's what they are requesting. Six quotes in that case doesn't make much sense to begin with.

I do not see those numbers for the preg_split. The 1MB of double quotes takes 2MB and a bit over 2MB are taken by preg_split. This is measuring just the preg_split call on 32 bits with memory_get_usage([true]). 32 bits vs 64 doesn't make a difference either. They just change the base size. The reported usage is much higher in 5.3 than 5.2 but that's probably due to the "Improve memory_get_usage() accuracy".

The alternate implementation of http://bug-attachment.wikimedia.org/attachment.cgi?id=6596 seems to be slightly more memory friendly. It may make sense to use it, at least where APC is available.

Why is the bold spanning comments?

Imagine </b><b> at the point where the bold begins...

That's pretty amazing. Maybe you have a special version of preg_split() which stores the string offsets without using any additional memory. I heard that PCRE was developing some special technology for using the normally-hidden 9th bit in every byte.

In my more ordinary PHP:

$ php eval.php
> $i = intval(1048576/3); $text = str_repeat("'' ", $i);

> $m = memory_get_usage();

> $arr = preg_split( "/(''+)/", $text, -1, PREG_SPLIT_DELIM_CAPTURE );

> print memory_get_usage()-$m;
138421072

400 bytes per match.

Look at r61525-r61528 when you wake up, Tim :)

...
+'''Bold''BoldItalic''''''Normal
+
+''Italic'''BoldItalic''''''Normal'''''
...

The second one looks odd to me; why the five quotes at the end? If you leave them away, you get the following test case

''Italic'''BoldItalic''''''Normal

that now results in

Italic'BoldItalicNormal

which is at least inconsistent with the case above (and also looks kind of counter-intuitive to me).

I think the reason for this behavior is that six quotes are now handled differently in the two passes. In the first pass they are counted as two bold markups, while in the second pass they are sometimes treated like a single quote followed by five quotes.

The five quotes at the end are for balancing.

''Italic'''BoldItalic''''''Normal'''''

would be equivalent to writing:

<i>Italic<b>BoldItalic</b><b>Normal</b></i>

Currently (pre-parser changes) it renders:

<i>Italic'</i>BoldItalic'<i><b>Normal</b></i>

which I find much more unlikely to be the desired result.

''Italic'''BoldItalic''''''Normal

is not balanced.

We have to guess what wanted the user. Old guess is

<i>Italic<b>BoldItalic'</b></i>Normal

and the new one

<i>Italic'</i>BoldItalic<b></b>Normal

I can easily add a rule to prefer the six quotes when choosing the ' to make part of the text.

<i>Italic<b>BoldItalic'</b></i>Normal

Is that what you are requesting?

I'm referring to the difference the new behavior introduced for the two cases

'''Bold''BoldItalic''''''Normal
''Italic'''BoldItalic''''''Normal

The current parser renders them as

<b>Bold<i>BoldItalic'</i></b>Normal
<i>Italic<b>BoldItalic'</b></i>Normal

while the new version would output

<b>Bold<i>BoldItalic'</i></b>Normal
<i>Italic'</i>BoldItalicNormal

If these cases are balanced or not, depends on the implementation. After all, I don't really see why treating six quotes as <b></b> should be more reasonable than treating them as '</b></i>. Suppose someone writes

''''''This is very important!''''''

The new version would just remove all quotes entirely, which could be quite surprising to the user :)

That's the issue, giving a reasonable solution. But whatever we choose, it will break something.

'''bold''''''bold''bolditalics'''''

rendered as

<b>bold</b><b>bold<i>bolditalics</i></b>

seems more reasonable to me than

'<i>bold'</i><b>bold<i>bolditalics</i></b>

Looking at six quote usage on enwiki, there are instances which are just typos [1] or vandalism.

Some cases wanted a <b></b>, also some templates with '''{{{1|}}}'''.

On others they wanted a quoted bold italic text, like Algol Expressions or Remnants. However, the output isn't right either since one quote is bold italic and the other isn't. It should either affect both or none. There are also instances were they wanted a Item's, which wouldn't be affected by this change (although the ' is put inside the formatting and should be outside).

We could make six quotes when they are around a word to make it bold italic placing extra ' in the inside, but i'm reluctant to such changes.

PS: Crappy links on this comment should show correctly when r61581 goes live.

Yes, I guess most of the instances are typos and such, however such mistakes are easier to spot if we output _something_, instead of just silently removing the quotes.

Whatever heuristic you choose, it should IMHO be consistent, in a way that the two cases I mentioned in my previous post work symmetrically. Moreover, the following should work the same:

'''Bold''BoldItalic''''''Normal
Foo. '''Bold''BoldItalic''''''Normal

But with the new version we get

<b>Bold<i>BoldItalic'</i></b>Normal
Foo. '<i>Bold</i>BoldItalicNormal

I have made on r61613 six quotes the first candidate when we have a quote too much.

This takes care of given samples. For this last one, output will be:

<b>Bold<i>BoldItalic'</i></b>Normal
Foo. <b>Bold<i>BoldItalic'</i></b>Normal

There will likely be differences for more complex lines though.