Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
646003002:
Introduced hash valued location markers in the spec (Closed)
|
Created:
6 years, 2 months ago by eernst Modified:
6 years, 2 months ago CC:
reviews_dartlang.org, ricow1 Visibility:
Public. |
DescriptionIntroduced hash valued location markers in the spec
Introduced support for adding SHA1 hash valued location markers
at several levels in the language specification, added long explanatory
comment at the end, added a script 'addlatexhash.dart' to normalize
spacing, remove comments, etc., in the spec, such that the hash values
are more robust than they would be with a direct usage of the spec.
The script passes the "dvi2tty test", that is, when the location markers
are empty, the resulting *.dvi files created from dartLangSpec.tex and
from the version processed by the script give rise to the same text via
dvi2tty, i.e., the script does not destroy the spec.
R=gbracha@google.com, ricow@google.com
Committed: https://code.google.com/p/dart/source/detail?r=41191
Patch Set 1 #
Total comments: 56
Patch Set 2 : Created spec location marker test, adjusted filter #
Total comments: 80
Patch Set 3 : Adjusted after review #
Total comments: 28
Patch Set 4 : Revised after 2nd review #
Messages
Total messages: 23 (3 generated)
eernst@google.com changed reviewers: + gbracha@google.com
This is the version of the spec that contains all the new stuff associated with
location markers, along with a filter 'addlatexhash.dart' that simplifies the
LaTeX source in ways that do not affect the output (and which will soon add the
actual hash values to all \LMHash{} commands, such that a spec with paragraph
markers can be created by piping dartLangSpec.tex through the filter before
latex'ing.
lgtm with comment https://codereview.chromium.org/646003002/diff/1/docs/language/dart.sty File docs/language/dart.sty (right): https://codereview.chromium.org/646003002/diff/1/docs/language/dart.sty#newco... docs/language/dart.sty:133: \definecolor{LMdim}{gray}{0.95} Weren't we going to make this totally white so it would be invisible?
ricow@google.com changed reviewers: + ricow@google.com
dbc, mostly style comments, but I really think you should use the repo version of the crypto package and not inline import the stuff you need. If you do that, you should make sure to add a tests inside the test directory that validates what you are doing (to avoid having trigger happy people simply remove it from our repo and move it to github) https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:1: // ---------------------------------------------------------------------- Add copyright header to this file https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:2: // File 'addlatexhash.dart' We don't normally have this as a header, just leave it out https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:9: // dart ../../tools/addlatexhash.dart < dartLangSpec.tex >tmp.tex If I where you I would simple use command line parameters instead, i.e., --input=dartLangSpec.tex --out=tmp.tex, the code in here becomes much nicer. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:288: var comment_all_re = new RegExp("^%"); camel case variables, these could be const? (in which case I would do uppercase names with underbars) https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:296: cut_match(line, match, {start_offset:0, end_offset:0, glue:""}) { Camel case method names, here and below https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:301: if (start < 0) start=0; space around = https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:302: if (end > len) end=len; space around = https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:315: return line.substring(0,match.start+offset) + glue; space around + https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:327: return line[line.length-1] == '\n'? "\n" : ""; space before ? https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:352: line = cut_match(line, match, glue:" "); always encapsulate loop and conditional blocks in {} if not on the same line (even if it is just one adjacent line) https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:356: // reduce sequences of >1 ws_only lines to 1, and reduce -> Reduce https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:357: // and sequences of >1 comment_only lines to 1 and and -> and https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:366: throw new Error("Bug, please report"); I would do Bug, please report to eernst@ https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:367: } move else if up on this line https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:385: } move else up here https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:431: you should add a real test that imports this file and runs the tests. This will not be run as part of our normal test runs https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:482: rl() => stdin.readLineSync( we don't normally shorten names, i.e., rl should be readLine https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:489: main () { As stated in the top this becomes much easier if you just have a file as input, you can do file.readAsLines (sync if you want) https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:490: var lines = new List(), line; I would do the definition of line on a separate line here https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:496: in_dart_code = true; block in {} https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:498: in_dart_code = false; block in {} https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:499: if (in_dart_code) lines.add(sisp_normalize(line)); always use {} blocks when you have anything but simple one line if statements (i.e., the only case where you should not use {} is when you have "if (foobar == barfoo) return true;"
Hello Gilad and Rico, @Rico, I have inserted a couple of questions in the comments (issues that weren't immediately fixable): How do I arrive at a reasonable design for the command line interface (--input etc.), and how/where do I create "real" test cases? I got some input from Johnni and others here, but these two issues were not trivial to resolve. Also, using the 'pub' version of the crypto package was not an option that came to mind for Kevin Moore last Friday when the issue came up --- I found crypto on github, and I couldn't find it as a standard package (maybe I just don't know enough about how to find it), and Kevin concluded that I would have to (and could choose to) copy/paste the code from github. Is there a portable (command line compatible) way to get access to this code without copy/pasting it? https://codereview.chromium.org/646003002/diff/1/docs/language/dart.sty File docs/language/dart.sty (right): https://codereview.chromium.org/646003002/diff/1/docs/language/dart.sty#newco... docs/language/dart.sty:133: \definecolor{LMdim}{gray}{0.95} On 2014/10/10 21:51:59, gbracha wrote: > Weren't we going to make this totally white so it would be invisible? Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:1: // ---------------------------------------------------------------------- On 2014/10/13 06:07:03, ricow1 wrote: > Add copyright header to this file Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:2: // File 'addlatexhash.dart' On 2014/10/13 06:07:03, ricow1 wrote: > We don't normally have this as a header, just leave it out Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:9: // dart ../../tools/addlatexhash.dart < dartLangSpec.tex >tmp.tex On 2014/10/13 06:07:03, ricow1 wrote: > If I where you I would simple use command line parameters instead, i.e., > --input=dartLangSpec.tex --out=tmp.tex, the code in here becomes much nicer. OK. But there are myriads of ways to define the keywords like 'input' and 'output' here, and it might be confusing for some users that there are no non-option arguments. E.g., it would be rather common for utilities to take the path to a file as a non-option argument, and then to handle an arbitrary number of arguments (by processing each of the files given as arguments), to expect a certain "type" of file (gcc accepts *.c and a few others, and each type of file gets its own specific treatment). So this is a can of worms, how do I arrive at a design that matches the expectations in this context? ;-) https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:288: var comment_all_re = new RegExp("^%"); On 2014/10/13 06:07:04, ricow1 wrote: > camel case variables, these could be const? (in which case I would do uppercase > names with underbars) Now using camel case, but 'const RegExp(..)' is not allowed so it cannot be const on the declaration, either. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:288: var comment_all_re = new RegExp("^%"); On 2014/10/13 06:07:04, ricow1 wrote: > camel case variables, these could be const? (in which case I would do uppercase > names with underbars) CamelCasing done. But it can't be const (can't use 'const RegExp(..)'). https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:296: cut_match(line, match, {start_offset:0, end_offset:0, glue:""}) { On 2014/10/13 06:07:03, ricow1 wrote: > Camel case method names, here and below Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:301: if (start < 0) start=0; On 2014/10/13 06:07:03, ricow1 wrote: > space around = Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:302: if (end > len) end=len; On 2014/10/13 06:07:04, ricow1 wrote: > space around = Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:315: return line.substring(0,match.start+offset) + glue; On 2014/10/13 06:07:03, ricow1 wrote: > space around + Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:327: return line[line.length-1] == '\n'? "\n" : ""; On 2014/10/13 06:07:03, ricow1 wrote: > space before ? Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:352: line = cut_match(line, match, glue:" "); On 2014/10/13 06:07:03, ricow1 wrote: > always encapsulate loop and conditional blocks in {} if not on the same line > (even if it is just one adjacent line) Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:352: line = cut_match(line, match, glue:" "); On 2014/10/13 06:07:03, ricow1 wrote: > always encapsulate loop and conditional blocks in {} if not on the same line > (even if it is just one adjacent line) Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:356: // reduce sequences of >1 ws_only lines to 1, and On 2014/10/13 06:07:03, ricow1 wrote: > reduce -> Reduce Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:356: // reduce sequences of >1 ws_only lines to 1, and On 2014/10/13 06:07:03, ricow1 wrote: > reduce -> Reduce Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:357: // and sequences of >1 comment_only lines to 1 On 2014/10/13 06:07:03, ricow1 wrote: > and and -> and Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:366: throw new Error("Bug, please report"); On 2014/10/13 06:07:04, ricow1 wrote: > I would do Bug, please report to eernst@ Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:385: } On 2014/10/13 06:07:03, ricow1 wrote: > move else up here Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:431: On 2014/10/13 06:07:03, ricow1 wrote: > you should add a real test that imports this file and runs the tests. This will > not be run as part of our normal test runs Need a bit more input on how to do this. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:482: rl() => stdin.readLineSync( On 2014/10/13 06:07:03, ricow1 wrote: > we don't normally shorten names, i.e., rl should be readLine Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:490: var lines = new List(), line; On 2014/10/13 06:07:03, ricow1 wrote: > I would do the definition of line on a separate line here Done. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:496: in_dart_code = true; On 2014/10/13 06:07:03, ricow1 wrote: > block in {} Already changed this to single line as a result of earlier comment, I assumed that would be comme il faut, too. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:496: in_dart_code = true; On 2014/10/13 06:07:03, ricow1 wrote: > block in {} Changed this to single line already when you described this style issue earlier. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:498: in_dart_code = false; On 2014/10/13 06:07:03, ricow1 wrote: > block in {} Same situation as l.496. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:499: if (in_dart_code) lines.add(sisp_normalize(line)); On 2014/10/13 06:07:04, ricow1 wrote: > always use {} blocks when you have anything but simple one line if statements > (i.e., the only case where you should not use {} is when you have "if (foobar == > barfoo) return true;" Ah, so you're saying that the presence of 'else' (including 'else if') already implies that we are no longer in single-line style, and braces must be used?
For the spec, only dart.sty was changed, to use white color. For addlatexhash.dart, many changes were made, as described inline. New file added: addlatexhash_test.dart, containing the testing parts of the previous addlatexhash.dart as well as a new Dart-based test doing the dvi2tty comparison that used to be a separate script.
mostly style https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... File tests/standalone/io/addlatexhash_test.dart (right): https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:12: final dartRootPath = "/" + pathList.join("/") + "/../../.."; you can do: import 'package:path/path.dart'; and use the path package to do the manipulation, in which case it will also work on windows (this will not) https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:15: // the standard output, otherwise report the failure comment is wrong, since this is not what we are doing here, we always return null. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:17: var output; you need to set output here, otherwise it will not be set in the standard case https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:27: // testing cutMatch Remove comment, obvious https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:30: // Code which is commented out: useful during debugging we don't do commented out code, not in production code and not in tests. It is impossible to maintain since when you change the code you will not change the comments. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:43: ? "OK" : throw "Failed in testCutMatch"; I would structure this differently. When this fails you have no idea which one of the above tests failed. You can easily just do the oneTestCutMatch calls on seperate lines and throw in oneTestCutMatch with a better error message https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:46: // testing sisp* functions same as above https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:50: // stdout.write("sispIsDart*: ${line}: ${expectation}\n"); no commented out code https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:61: ? "OK" : throw "Failed in testSisp"; Same comment as above, throw in the oneTestSisp function https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:64: testNoChange() { Explicitly state that this is _not_ run as part of normal testing https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:68: // actions this is not actions? (also, we don't normally name sections like that, it does not add much value) https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:69: final tmpDirPath = tmpDir.path; for all of this and the below also use the path package https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:70: final specDirPath = dartRootPath + "/docs/language"; as stated above, use the path package. That said, I would normally use string interpolation in cases like this https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:100: for (var i=0; i<5; i++) { space around = and < https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:104: checkAction(Process.runSync("cp", [styPath, tmpDirPath]), new File('styPath').copySync('$tmpDirPath/$styFileName'); https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:106: for (var i=0; i<5; i++) { space around = and < https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:11: // dart ../../tools/addlatexhash.dart < dartLangSpec.tex >tmp.tex outdated comment, input and output is now taken as paramters https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:13: // This will yield a variant tmp.tex of the language specification with tmp.tex -> whatever you call the output file aboe https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:14: // hash values filled in. For more details, please check the language well, not yet, add a todo. Maybe also state that we remove the comments we can https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:28: // regexps obvious, remove comment https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:38: cutMatch(line, match, {startOffset:0, endOffset:0, glue:""}) { space after : https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:48: cutRegexp(line, re, {startOffset:0, endOffset:0, glue:""}) { space after : https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:52: glue: glue); indendtation https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:52: glue: glue); indentation https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:55: cutFromMatch(line, match, {offset:0, glue:""}) { space after : https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:60: cutFromRegexp(line, re, {offset:0, glue:""}) { space after : https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:61: return cutFromMatch(line, re.firstMatch(line), offset:offset, glue:glue); space after : https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:69: return line[line.length-1] == '\n' ? "\n" : ""; be consistent in using either ' or " for strings in the whole file also, space around - in the array index calculation https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:84: return cutRegexp(line, commentRe, startOffset:2); space after : https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:91: line = cutRegexp(line, whitespaceLeadingRe, endOffset:-1); space after : https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:94: line = cutMatch(line, match, glue:" "); space after : https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:102: var oldlines = lines; why are we doing this name mangling: oldlines lines; lines = new List(); We now reuse the input parameter, which is pretty strange https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:133: // blank line block continues, do not add 'line' do we really want an else clause that does nothing https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:143: // comment line block continues, do not add 'line' same as above https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:145: } else /* !afterBlankLines && !afterCommentLines */ { you have comments after the line in all other places in this file, also do that here https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:176: // main obvious, remove comment https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:200: newLines.add(sispNormalize(line + "\n")); maybe remove the "\n" here and join on it when writing the output file
Almost all issues resolved as described in comments, but not these two (details inline): (1) Can I trust the --package-root for all testing environments? (2) Where does code go if it is useful only for debugging, but still maybe non-trivial to reinvent? https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... File tests/standalone/io/addlatexhash_test.dart (right): https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:12: final dartRootPath = "/" + pathList.join("/") + "/../../.."; On 2014/10/14 06:09:10, ricow1 wrote: > you can do: > import 'package:path/path.dart'; > and use the path package to do the manipulation, in which case it will also work > on windows (this will not) OK, required a '--package-root=$dartroot/out/ReleaseIA32/packages' where dartroot is a long beast. Seems that tools/test.py provides a similar argument (no "cannot open" raised when using test.py), so my assumption is that a test can rely on having such a package-root. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:15: // the standard output, otherwise report the failure On 2014/10/14 06:09:10, ricow1 wrote: > comment is wrong, since this is not what we are doing here, we always return > null. .. or throw; but the comment should be much more true now. ;) https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:17: var output; On 2014/10/14 06:09:10, ricow1 wrote: > you need to set output here, otherwise it will not be set in the standard case Forgot to resolve the case: Is a ProcessResult even in a state where 'stdout' etc can be read when the process fails? The API does not mention this, https://api.dartlang.org/apidocs/channels/dev/dartdoc-viewer/dart-io.ProcessR..., but it seems that we can read it. So I just do that up front. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:27: // testing cutMatch On 2014/10/14 06:09:10, ricow1 wrote: > Remove comment, obvious Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:30: // Code which is commented out: useful during debugging On 2014/10/14 06:09:10, ricow1 wrote: > we don't do commented out code, not in production code and not in tests. It is > impossible to maintain since when you change the code you will not change the > comments. Sounds nice and clean! But then what's the right way to preserve code that may be needed now and then (typically: during debugging), but should not bog down deployed systems? Always reinvent, some kind of #ifdef, ..? https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:43: ? "OK" : throw "Failed in testCutMatch"; On 2014/10/14 06:09:10, ricow1 wrote: > I would structure this differently. When this fails you have no idea which one > of the above tests failed. You can easily just do the oneTestCutMatch calls on > seperate lines and throw in oneTestCutMatch with a better error message Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:46: // testing sisp* functions On 2014/10/14 06:09:10, ricow1 wrote: > same as above Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:50: // stdout.write("sispIsDart*: ${line}: ${expectation}\n"); On 2014/10/14 06:09:10, ricow1 wrote: > no commented out code Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:61: ? "OK" : throw "Failed in testSisp"; On 2014/10/14 06:09:10, ricow1 wrote: > Same comment as above, throw in the oneTestSisp function Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:64: testNoChange() { On 2014/10/14 06:09:11, ricow1 wrote: > Explicitly state that this is _not_ run as part of normal testing Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:68: // actions On 2014/10/14 06:09:10, ricow1 wrote: > this is not actions? (also, we don't normally name sections like that, it does > not add much value) Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:69: final tmpDirPath = tmpDir.path; On 2014/10/14 06:09:10, ricow1 wrote: > for all of this and the below also use the path package Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:70: final specDirPath = dartRootPath + "/docs/language"; On 2014/10/14 06:09:10, ricow1 wrote: > as stated above, use the path package. That said, I would normally use string > interpolation in cases like this Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:100: for (var i=0; i<5; i++) { On 2014/10/14 06:09:10, ricow1 wrote: > space around = and < Done. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:104: checkAction(Process.runSync("cp", [styPath, tmpDirPath]), On 2014/10/14 06:09:10, ricow1 wrote: > new File('styPath').copySync('$tmpDirPath/$styFileName'); Done, except using plain styPath. https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:106: for (var i=0; i<5; i++) { On 2014/10/14 06:09:11, ricow1 wrote: > space around = and < Done.
https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:11: // dart ../../tools/addlatexhash.dart < dartLangSpec.tex >tmp.tex On 2014/10/14 06:09:12, ricow1 wrote: > outdated comment, input and output is now taken as paramters Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:13: // This will yield a variant tmp.tex of the language specification with On 2014/10/14 06:09:11, ricow1 wrote: > tmp.tex -> whatever you call the output file aboe Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:14: // hash values filled in. For more details, please check the language On 2014/10/14 06:09:12, ricow1 wrote: > well, not yet, add a todo. Maybe also state that we remove the comments we can Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:28: // regexps On 2014/10/14 06:09:12, ricow1 wrote: > obvious, remove comment Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:38: cutMatch(line, match, {startOffset:0, endOffset:0, glue:""}) { On 2014/10/14 06:09:12, ricow1 wrote: > space after : Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:48: cutRegexp(line, re, {startOffset:0, endOffset:0, glue:""}) { On 2014/10/14 06:09:11, ricow1 wrote: > space after : Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:52: glue: glue); On 2014/10/14 06:09:11, ricow1 wrote: > indentation Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:52: glue: glue); On 2014/10/14 06:09:11, ricow1 wrote: > indentation Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:55: cutFromMatch(line, match, {offset:0, glue:""}) { On 2014/10/14 06:09:12, ricow1 wrote: > space after : Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:60: cutFromRegexp(line, re, {offset:0, glue:""}) { On 2014/10/14 06:09:12, ricow1 wrote: > space after : Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:61: return cutFromMatch(line, re.firstMatch(line), offset:offset, glue:glue); On 2014/10/14 06:09:12, ricow1 wrote: > space after : Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:69: return line[line.length-1] == '\n' ? "\n" : ""; On 2014/10/14 06:09:11, ricow1 wrote: > be consistent in using either ' or " for strings in the whole file > also, space around - in the array index calculation Using '"' for strings, "'" for imports. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:84: return cutRegexp(line, commentRe, startOffset:2); On 2014/10/14 06:09:11, ricow1 wrote: > space after : Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:91: line = cutRegexp(line, whitespaceLeadingRe, endOffset:-1); On 2014/10/14 06:09:12, ricow1 wrote: > space after : Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:94: line = cutMatch(line, match, glue:" "); On 2014/10/14 06:09:12, ricow1 wrote: > space after : Done. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:102: var oldlines = lines; On 2014/10/14 06:09:11, ricow1 wrote: > why are we doing this name mangling: > oldlines lines; > lines = new List(); > > We now reuse the input parameter, which is pretty strange Was thinking "transforming lines several times" (where 'oldlines' would repeatedly be the previous form, and 'lines' would accumulate the transformed text). Now using the slightly more functional style where the transformed text gets a new name 'newLines'. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:133: // blank line block continues, do not add 'line' On 2014/10/14 06:09:12, ricow1 wrote: > do we really want an else clause that does nothing Worried about performance? Not sure about the costs. Where else would you put an invariant that justifies leaving this case as a noop (rather than just a forgotten case)? https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:143: // comment line block continues, do not add 'line' On 2014/10/14 06:09:12, ricow1 wrote: > same as above Same issue, to be resolved together. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:145: } else /* !afterBlankLines && !afterCommentLines */ { On 2014/10/14 06:09:12, ricow1 wrote: > you have comments after the line in all other places in this file, also do that > here This is actually a bit different, because this comment contains the exact code that would have occurred here if I had used a style where all cases are spelled out (and hence exactly one of them applies at runtime). But maybe the best treatment of this situation is actually to make it real code, in an assertion, such that we catch situations where the other cases are edited, or null is encountered, etc. Did that. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:176: // main On 2014/10/14 06:09:12, ricow1 wrote: > obvious, remove comment It wasn't really meant to be unobvious, it should make the file as a whole more navigable, because of "section" dividers like the line here. Don't we use any such thing? https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:200: newLines.add(sispNormalize(line + "\n")); On 2014/10/14 06:09:12, ricow1 wrote: > maybe remove the "\n" here and join on it when writing the output file Would look better, but I would need to change many other parts of the algorithm, because lines with an eol char have a different effect than lines without an eol char (TeX and LaTeX are very peculiar with newlines).
ricow@google.com changed reviewers: + lrn@google.com
lgtm +lrn if you want input on the regexps and text manipulation (I did not look closely at that) https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... File tests/standalone/io/addlatexhash_test.dart (right): https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:12: final dartRootPath = "/" + pathList.join("/") + "/../../.."; On 2014/10/14 15:06:22, eernst wrote: > On 2014/10/14 06:09:10, ricow1 wrote: > > you can do: > > import 'package:path/path.dart'; > > and use the path package to do the manipulation, in which case it will also > work > > on windows (this will not) > > OK, required a '--package-root=$dartroot/out/ReleaseIA32/packages' where > dartroot is a long beast. Seems that tools/test.py provides a similar argument > (no "cannot open" raised when using test.py), so my assumption is that a test > can rely on having such a package-root. Yep, that is automatically passed to the vm for the tests https://codereview.chromium.org/646003002/diff/130001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:30: // Code which is commented out: useful during debugging On 2014/10/14 15:06:22, eernst wrote: > On 2014/10/14 06:09:10, ricow1 wrote: > > we don't do commented out code, not in production code and not in tests. It is > > impossible to maintain since when you change the code you will not change the > > comments. > > Sounds nice and clean! > > But then what's the right way to preserve code that may be needed now and then > (typically: during debugging), but should not bog down deployed systems? Always > reinvent, some kind of #ifdef, ..? well, normally when you need to debug the info you need is so specialized that you might as well write it again. This will take you 30 seconds. If you really want debugging info (which you rarely want in a test) - I would have a top level flag printDebugInfo and condition on that in the code - defaulting to false but easily turned on, we have that for some of our test infrastructure code https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:133: // blank line block continues, do not add 'line' On 2014/10/14 15:53:25, eernst wrote: > On 2014/10/14 06:09:12, ricow1 wrote: > > do we really want an else clause that does nothing > > Worried about performance? Not sure about the costs. Where else would you put > an invariant that justifies leaving this case as a noop (rather than just a > forgotten case)? I would just have it as a comment, but this is fine, just leave it https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:145: } else /* !afterBlankLines && !afterCommentLines */ { On 2014/10/14 15:53:25, eernst wrote: > On 2014/10/14 06:09:12, ricow1 wrote: > > you have comments after the line in all other places in this file, also do > that > > here > > This is actually a bit different, because this comment contains the exact code > that would have occurred here if I had used a style where all cases are spelled > out (and hence exactly one of them applies at runtime). But maybe the best > treatment of this situation is actually to make it real code, in an assertion, > such that we catch situations where the other cases are edited, or null is > encountered, etc. Did that. Acknowledged. https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:176: // main On 2014/10/14 15:53:24, eernst wrote: > On 2014/10/14 06:09:12, ricow1 wrote: > > obvious, remove comment > > It wasn't really meant to be unobvious, it should make the file as a whole more > navigable, because of "section" dividers like the line here. Don't we use any > such thing? I never do, if I want to structure something together logically, I will put it in a class. The problem with doing it like this is that over time people will add code in sections where it does not belong, the comments will be outdated/wrong, and people don use a comment syntax so it is hard to do anything with command line wise. If the code is self explanatory I never add a comment. There are a few cases where section headers like this make sense, e.g., if you have code that is preprocessed before compilation https://codereview.chromium.org/646003002/diff/130001/tools/addlatexhash.dart... tools/addlatexhash.dart:200: newLines.add(sispNormalize(line + "\n")); On 2014/10/14 15:53:24, eernst wrote: > On 2014/10/14 06:09:12, ricow1 wrote: > > maybe remove the "\n" here and join on it when writing the output file > > Would look better, but I would need to change many other parts of the algorithm, > because lines with an eol char have a different effect than lines without an eol > char (TeX and LaTeX are very peculiar with newlines). Acknowledged. https://codereview.chromium.org/646003002/diff/220001/tests/standalone/io/add... File tests/standalone/io/addlatexhash_test.dart (right): https://codereview.chromium.org/646003002/diff/220001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:18: var output = result.stdout; there is not really any good reason to do this assignment, just use result.stdio below in the the two places, makes code easier to read
Comments on string manipulation only. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:9: // dart ../../tools/addlatexhash.dart < dartLangSpec.tex >tmp.tex I'd favor taking the input name as unnnamed argument, using stdin of none are provided (or if passed the "-" argument). Optionally accept an "-o outputfile" or "--out outputfile" for the output, defaulting to stdout if none provided. Only handle one file. Otherwise you need a standard way of renaming inputs or storing with same name somewhere else (-i.tmp or --output-dir=someotherdir). For now, one file should be enough. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:306: cut_regexp(line, re, {start_offset:0, end_offset:0, glue:""}) { Is this function used? https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:323: is_comment_only(line) => comment_all_re.firstMatch(line) != null; This would be the non-regexp version: => line.startsWith("%"); https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:33: final commentAllRe = new RegExp("^%"); Using a RegExp for this is overkill, just do string.startsWith("%"). Two-letter acronyms can be fully upper-case in names, so maybe "commentAllRE" (but that's a matter of taste). https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:34: final commentRe = new RegExp("[^\\\\]%[^\\n]*"); I recommend using raw strings for RegExp sources: final commentRE = new RegExp(r"[^\\]%[^\n]"); Much more readable! :) https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:37: final whitespaceRe = new RegExp("[ \\t][ \\t]+"); Shorter regexp possible: final whitespaceRE = new RegExp(r"[ \t]{2,}"); https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:68: isCommentOnly(line) => commentAllRe.firstMatch(line) != null; This would be the non-regexp version: => line.startsWith("%"); Avoiding unnecessary RegExps is a pet peeve. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:70: justEol(line) { I'd prefer return types, and parameter types, in general. So: String justEol(String line) { https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:72: return line[line.length-1] == "\n" ? "\n" : ""; return line.endsWith("\n") ? "\n" : ""; https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:87: return cutRegexp(line, commentRe, startOffset: 2); Doesn't this loose the trailing '\n'? https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:92: normalizeWhitespace(line) { To remove leading WS (including empty lines), try: var trimLine = line.trimLeft(); if (trimLine.isEmpty) return justEol(line); line = trimLine; https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:98: } Maybe just: line = line.replaceAll(whitespaceRe, " "); https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:152: if (isCommentOnly(line)) afterCommentLines = true; Maybe: if (isCommentOnly(line)) { afterCOmmentLines = true; } else { if (isWsOnly(line)) afterblankLines = true; newLines.add(line); } (I'm just a simple programmer, seeing assignments to a variable followed by a condition on that variable confuses and frightens me). https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:169: final dartCodeBeginRe = new RegExp("^\\s*\\\\begin{dartCode}"); Need to escape '{' and '}' in RegExp: new RegExp(r"^\s*\\begin\{dartCode\}") and ditto below.
https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:306: cut_regexp(line, re, {start_offset:0, end_offset:0, glue:""}) { Is this comment still here? Yes it is. Should it be? No. So, please ignore.
https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:9: // dart ../../tools/addlatexhash.dart < dartLangSpec.tex >tmp.tex On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > I'd favor taking the input name as unnnamed argument, using stdin of none are > provided (or if passed the "-" argument). > > Optionally accept an "-o outputfile" or "--out outputfile" for the output, > defaulting to stdout if none provided. > > Only handle one file. Otherwise you need a standard way of renaming inputs or > storing with same name somewhere else (-i.tmp or --output-dir=someotherdir). > For now, one file should be enough. That is already the case, you are looking at the old patchset
https://codereview.chromium.org/646003002/diff/220001/tests/standalone/io/add... File tests/standalone/io/addlatexhash_test.dart (right): https://codereview.chromium.org/646003002/diff/220001/tests/standalone/io/add... tests/standalone/io/addlatexhash_test.dart:18: var output = result.stdout; On 2014/10/15 08:29:05, ricow1 wrote: > there is not really any good reason to do this assignment, just use result.stdio > below in the the two places, makes code easier to read OK, I did that because I envision (1) result.stdout may be/become a getter, so how long does that take?, and (2) what if we get two different results from the two occurrences? It's just a habit of simplifying the semantics when possible. But I've changed it back to use two occurrences of result.stdout because no execution will actually see both evaluations. In short: Done. ;)
Thanks for the clarification, no changes were made in relation to this review.
Comments to Lasse. This was the previous patch set, but a couple of issues are equally relevant to the new version, so I commented on them here. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:9: // dart ../../tools/addlatexhash.dart < dartLangSpec.tex >tmp.tex On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > I'd favor taking the input name as unnnamed argument, using stdin of none are > provided (or if passed the "-" argument). > > Optionally accept an "-o outputfile" or "--out outputfile" for the output, > defaulting to stdout if none provided. > > Only handle one file. Otherwise you need a standard way of renaming inputs or > storing with same name somewhere else (-i.tmp or --output-dir=someotherdir). > For now, one file should be enough. Actually, using -o for the output and default to stdout would be very much in the style of many traditional UN*X command line utilities (and accepting multiple files could then work like 'cat', just combining the processed input files and sending them to the same output destination). In that sense it would be nicer (more obvious to some users). But 'cp' offers prior art on the '<command> <src> <dst>' format, so I guess it's not too bad to keep the current format (<addlatexhash> <inputfile> <outputfile>). So I haven't changed anything in this respect. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:323: is_comment_only(line) => comment_all_re.firstMatch(line) != null; On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > This would be the non-regexp version: => line.startsWith("%"); Done. Used to have a slightly more general commentAllRe (allowing leading white space), but that turned out to be unnecessary. https://codereview.chromium.org/646003002/diff/1/tools/addlatexhash.dart#newc... tools/addlatexhash.dart:367: } On 2014/10/13 06:07:04, ricow1 wrote: > move else if up on this line Done.
Comments to Lasse. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:33: final commentAllRe = new RegExp("^%"); On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > Using a RegExp for this is overkill, just do string.startsWith("%"). > > Two-letter acronyms can be fully upper-case in names, so maybe "commentAllRE" > (but that's a matter of taste). Looks more meaningful with "RE". Done. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:34: final commentRe = new RegExp("[^\\\\]%[^\\n]*"); On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > I recommend using raw strings for RegExp sources: > > final commentRE = new RegExp(r"[^\\]%[^\n]"); > > Much more readable! :) Indeed; added the final "*", done. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:37: final whitespaceRe = new RegExp("[ \\t][ \\t]+"); On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > Shorter regexp possible: > final whitespaceRE = new RegExp(r"[ \t]{2,}"); Done. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:68: isCommentOnly(line) => commentAllRe.firstMatch(line) != null; On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > This would be the non-regexp version: => line.startsWith("%"); > > Avoiding unnecessary RegExps is a pet peeve. Arg null is treated also as an error by firstMatch, so: done. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:70: justEol(line) { On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > I'd prefer return types, and parameter types, in general. > So: > String justEol(String line) { > I decided to use a typeless style in this particular case because I spent so much time working on types, so that's the "missing case". But I get the impression that there is a "type everything" or at least "type the interface parts" rule in this context? Haven't made this change yet. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:72: return line[line.length-1] == "\n" ? "\n" : ""; On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > return line.endsWith("\n") ? "\n" : ""; Done. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:87: return cutRegexp(line, commentRe, startOffset: 2); On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > Doesn't this loose the trailing '\n'? No, commentRE avoids matching a trailing '\n', so an '\n' at the end of the line will be preserved, and absence of '\n' at the end will also be preserved. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:92: normalizeWhitespace(line) { On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > To remove leading WS (including empty lines), try: > var trimLine = line.trimLeft(); > if (trimLine.isEmpty) return justEol(line); > line = trimLine; Done. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:98: } On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > Maybe just: > line = line.replaceAll(whitespaceRe, " "); Entire method much nicer now! Done. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:152: if (isCommentOnly(line)) afterCommentLines = true; On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > Maybe: > > if (isCommentOnly(line)) { > afterCOmmentLines = true; > } else { > if (isWsOnly(line)) afterblankLines = true; > newLines.add(line); > } > > (I'm just a simple programmer, seeing assignments to a variable followed by a > condition on that variable confuses and frightens me). Actually that was because of the following pattern, which is used in all the cases here: detect state (conditions of if-tree) // in each case update state variables act according to situation (which case, possibly new state) Changed the code to make this pattern a bit more obvious. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:169: final dartCodeBeginRe = new RegExp("^\\s*\\\\begin{dartCode}"); On 2014/10/15 09:13:17, Lasse Reichstein Nielsen wrote: > Need to escape '{' and '}' in RegExp: > > new RegExp(r"^\s*\\begin\{dartCode\}") > > and ditto below. OK. Note that we get no exceptions for this one, though: var re = new RegExp("some{other}thing"); if (re.firstMatch(" some{other}thing ") == null) { throw "Braces must be escaped in REs to mean braces!"; } so the braces stand for themselves without escaping in that case, presumably because the repeat count syntax failed to parse. Changed it anyway, because that's probably a temporary quirk of RegExp. Create an issue on that? If so, where?
https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:34: final commentRe = new RegExp("[^\\\\]%[^\\n]*"); Also just noticed that as a regexp, "[^n]" is equivalent to ".", so it can get even smaller: r"[^\\]%.*" Yey! https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:169: final dartCodeBeginRe = new RegExp("^\\s*\\\\begin{dartCode}"); RegExp in browsers have traditionally been very forgiving (as in: If Netscape allowed something, we better keep allowing it, whether it's in the spec or not). Most RegExp syntax errors are silently error-corrected by treating the invalid character as a literal. There's a good chance that PCRE (currently used by the VM) has done the same. I don't think we'll get dart2js to not use V8's RegExp, so it'll probably keep working there. I'd file a bug for the VM, and try to keep it from getting rolled into the "VM RegExps are just wrong" bug somehow. Maybe wait until the new RegExp implementation lands - it will likely use the same irregexp parser used by V8, so it will have the same forgivances. I'd love to weed those out once and for all. No need to be backwards compatible with non-standard JS code in new language!
A couple of comments to Lasse. https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart File tools/addlatexhash.dart (right): https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:34: final commentRe = new RegExp("[^\\\\]%[^\\n]*"); On 2014/10/15 14:09:57, Lasse Reichstein Nielsen wrote: > Also just noticed that as a regexp, "[^n]" is equivalent to ".", so it can get > even smaller: r"[^\\]%.*" > Yey! Cool! Done + Added a comment, just in case someone else will ever forget that . does not match \n. ;) https://codereview.chromium.org/646003002/diff/220001/tools/addlatexhash.dart... tools/addlatexhash.dart:169: final dartCodeBeginRe = new RegExp("^\\s*\\\\begin{dartCode}"); On 2014/10/15 14:09:57, Lasse Reichstein Nielsen wrote: > RegExp in browsers have traditionally been very forgiving (as in: If Netscape > allowed something, we better keep allowing it, whether it's in the spec or not). > Most RegExp syntax errors are silently error-corrected by treating the invalid > character as a literal. There's a good chance that PCRE (currently used by the > VM) has done the same. > > I don't think we'll get dart2js to not use V8's RegExp, so it'll probably keep > working there. I'd file a bug for the VM, and try to keep it from getting rolled > into the "VM RegExps are just wrong" bug somehow. Maybe wait until the new > RegExp implementation lands - it will likely use the same irregexp parser used > by V8, so it will have the same forgivances. I'd love to weed those out once and > for all. No need to be backwards compatible with non-standard JS code in new > language! > > What's the smart way to install a wakeup call to act when that new RegExp implementation arrives? We cannot create an issue now, supposedly.
Message was sent while issue was closed.
Committed patchset #4 (id:240001) manually as 41191 (presubmit successful).
Message was sent while issue was closed.
TBR Lasse, you raised an issue at the end (about the "VM RegExps are just wrong" bug). You said 'Maybe wait until the new RegExp implementation lands', and I asked for some kind of trigger such that we would be notified at a relevant time. If this is important then we should probably take it elsewhere, so I dcommitted the code and just left this "TBR" reminder. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
