r87936 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r87935‎ | r87936 | r87937 >
Date:22:29, 12 May 2011
Author:thenub314
Status:resolved (Comments)
Tags:math 
Comment:
Corrects a small bug causing <math>\sin</math> to return an unknown
function error
Modified paths:
  • /trunk/extensions/Math/math/lexer.mll (modified) (history)

Diff [purge]

Index: trunk/extensions/Math/math/lexer.mll
——@@ -13,7 +13,7 @@
1414 let delimiter_uf_op = ['/' '|']
1515 let boxchars = ['0'-'9' 'a'-'z' 'A'-'Z' '+' '-' '*' ',' '=' '(' ')' ':' '/' ';' '?' '.' '!' '\'' '`' ' ' '\128'-'\255']
1616 let aboxchars = ['0'-'9' 'a'-'z' 'A'-'Z' '+' '-' '*' ',' '=' '(' ')' ':' '/' ';' '?' '.' '\'' '`' '!' ' ']
17 -let latex_function_names = "arccos"| "arcsin" | "arctan" | "arg" | "cos" | "cosh" | "cot" | "coth" | "csc"| "deg" | "det" | "dim" | "exp" | "gcd" | "hom" | "inf" | "ker" | "lg" | "lim" | "liminf" | "limsup" | "ln" | "log" | "max" | "min" | "Pr" | "sec" | "sin" | "sinh" | "sup" | "tan" | "tanh"
 17+let latex_function_names = "arccos" | "arcsin" | "arctan" | "arg" | "cos" | "cosh" | "cot" | "coth" | "csc"| "deg" | "det" | "dim" | "exp" | "gcd" | "hom" | "inf" | "ker" | "lg" | "lim" | "liminf" | "limsup" | "ln" | "log" | "max" | "min" | "Pr" | "sec" | "sin" | "sinh" | "sup" | "tan" | "tanh"
1818 let mediawiki_function_names = "arccot" | "arcsec" | "arccsc" | "sgn" | "sen"
1919
2020 rule token = parse
——@@ -56,7 +56,6 @@
5757 | "-" { let str = Lexing.lexeme lexbuf in LITERAL (MHTMLABLEC (FONT_UFH,"-"," &minus; ",MO,str))}
5858 | literal_uf_op { let str = Lexing.lexeme lexbuf in LITERAL (MHTMLABLEC (FONT_UFH, str," "^str^" ",MO,str)) }
5959 | delimiter_uf_op { let str = Lexing.lexeme lexbuf in DELIMITER (MHTMLABLEC (FONT_UFH, str," "^str^" ",MO,str)) }
60 - | "\\" alpha + { Texutil.find (Lexing.lexeme lexbuf) }
6160 | "\\sqrt" space * "[" { FUN_AR1opt "\\sqrt" }
6261 | "\\xleftarrow" space * "[" { Texutil.tex_use_ams(); FUN_AR1opt "\\xleftarrow" }
6362 | "\\xrightarrow" space * "[" { Texutil.tex_use_ams(); FUN_AR1opt "\\xrightarrow" }
——@@ -79,6 +78,7 @@
8079 "\\operatorname{" ^ name ^ "}\\{", name ^ "{", MF, name, "{"))) }
8180 | "\\" (mediawiki_function_names as name) space *
8281 { (Texutil.tex_use_ams(); LITERAL (MHTMLABLEC(FONT_UFH,"\\operatorname{" ^ name ^ "}", name ^ "&nbsp;", MF, name))) }
 82+ | "\\" alpha + { Texutil.find (Lexing.lexeme lexbuf) }
8383 | "\\," { LITERAL (HTMLABLE (FONT_UF, "\\,","&nbsp;")) }
8484 | "\\ " { LITERAL (HTMLABLE (FONT_UF, "\\ ","&nbsp;")) }
8585 | "\\;" { LITERAL (HTMLABLE (FONT_UF, "\\;","&nbsp;")) }

Follow-up revisions

RevisionCommit summaryAuthorDate
r96990Revert changes to texvc that provide no test cases or examples of what they'r...brion19:00, 13 September 2011
r96993MFT r96990: provisional revert of texvc changes that don't come with any test...brion19:07, 13 September 2011
r97034* (bug 6722) Spacing fixes for math functions with/without parens...brion00:49, 14 September 2011

Comments

#Comment by Bawolff (talk | contribs)   23:03, 12 May 2011

<math>\sin foo32</math> still seems to fail. However I can confirm both <math>\sin</math> and <math>\sin{} foo32</math> seem to work fine

#Comment by Thelema314 (talk | contribs)   13:31, 29 August 2011

The major component of this change is lowering the match priority of "\\foo" patterns for unknown foo below other "\\" patterns (such as "\\sqrt"). This seems like a good step forward, as the current code seems to have no possibility of matching | "\\sqrt" through | "\\" (mediawiki_function_names as name)

#Comment by Brion VIBBER (talk | contribs)   01:05, 8 September 2011

If I understand correctly per [1], in case of multiple matches the longest match will apply... so the conflict would likely have been with the rule "\\" (latex_function_names as name) space *, which for eg "\sqrt" or "\sin" will match equally.

Fix makes sense to me now :DD

#Comment by Thelema314 (talk | contribs)   01:34, 8 September 2011

Oops, I forgot that this was in a lexer (which has longest match semantics for match cases, with ties broken to the first rule) as opposed to the usual OCaml matching construct, which has first-match semantics (and no concept of the longest match).

#Comment by Brion VIBBER (talk | contribs)   18:48, 8 September 2011

Still the right fix though :D

#Comment by Brion VIBBER (talk | contribs)   19:04, 13 September 2011

Provisionally reverted in r96990 pending test cases.

#Comment by Brion VIBBER (talk | contribs)   00:51, 14 September 2011

Reapplied in r97034 along with test cases & some friends.

Status & tagging log