Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1737623002:
[Interpreter] Multiple input files for generate-bytecode-expectations. (Closed)
|
Created:
4 years, 9 months ago by Stefano Sanfilippo Modified:
4 years, 9 months ago CC:
v8-reviews_googlegroups.com, oth, rmcilroy Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Interpreter] Multiple input files for generate-bytecode-expectations.
When operating in --rebaseline mode, each of the files will be updated.
In --raw-js mode, all the expectations will be written to the same file.
In default mode no more than one input file is accepted.
On POSIX systems, --rebaseline will autodiscover golden files when run
from the project root and no input file is provided.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/e039f63a4f809b940f67498b85749be31fd8b82a
Cr-Commit-Position: refs/heads/master@{#34324}
Patch Set 1 #
Total comments: 8
Patch Set 2 : ERROR reporting macro. #
Total comments: 1
Patch Set 3 : Drop "Processing foo.js" message. #Patch Set 4 : Fix compiler errors on Mac OS X. #Patch Set 5 : ERROR => REPORT_ERROR to solve MSVC macro redefinition error. #Patch Set 6 : Add --verbose flag and be silent otherwise. #Patch Set 7 : Unifying Do* functions. #
Total comments: 5
Patch Set 8 : Clarify how main() works. #Patch Set 9 : Wrap duplicated code in a function. #Patch Set 10 : Fix the same Mac OS X compiler error, again. #Patch Set 11 : Golden files autodiscovery on POSIX + restore flags after use. #Patch Set 12 : Assume running from V8 project root. #
Total comments: 2
Patch Set 13 : Use strcmp in EndsWith. #Patch Set 14 : EndsWith => StrEndsWith for uniformity with other string functions. #Patch Set 15 : Reword POSIX error message. #Patch Set 16 : Remove unneeded special case. #Messages
Total messages: 59 (24 generated)
ssanfilippo@chromium.org changed reviewers: + oth@chromium.org, rmcilroy@chromium.org
The subject says pretty much everything :) PTAL.
A couple of minor comments, but looks good here. lgtm. https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:456: std::cerr << "ERROR: Could not open '" << input_filename These ERROR messages could all be reported by a common routine to avoid duplicating messages. https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:476: std::cerr << "Reading " << input_filename << '\n'; This looks like left over debugging.
https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:456: std::cerr << "ERROR: Could not open '" << input_filename On 2016/02/25 13:12:38, oth wrote: > These ERROR messages could all be reported by a common routine to avoid > duplicating messages. Done. https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:476: std::cerr << "Reading " << input_filename << '\n'; On 2016/02/25 13:12:38, oth wrote: > This looks like left over debugging. Actually it's not, I thought it could give some feedback about the progress while the tool processes all the golden files. If you think it's just noise, I can remove it.
https://codereview.chromium.org/1737623002/diff/20001/test/cctest/interpreter... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/20001/test/cctest/interpreter... test/cctest/interpreter/generate-bytecode-expectations.cc:21: #define ERROR(MESSAGE) (((std::cerr << "ERROR: ") << MESSAGE) << '\n') Using a macro instead of a function allows the following syntax: ERROR("Foo is not a " << bar << " baz.");
Description was changed from ========== [Interpreter] Multiple input files for generate-bytecode-expectations. When operating in --rebaseline mode, each of the files will be updated. In --raw-js, all the expectations will be written to the same file. In default mode no more than one input file is accepted. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Multiple input files for generate-bytecode-expectations. When operating in --rebaseline mode, each of the files will be updated. In --raw-js mode, all the expectations will be written to the same file. In default mode no more than one input file is accepted. BUG=v8:4280 LOG=N ==========
On 2016/02/25 13:23:56, ssanfilippo wrote: > https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... > File test/cctest/interpreter/generate-bytecode-expectations.cc (right): > > https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... > test/cctest/interpreter/generate-bytecode-expectations.cc:456: std::cerr << > "ERROR: Could not open '" << input_filename > On 2016/02/25 13:12:38, oth wrote: > > These ERROR messages could all be reported by a common routine to avoid > > duplicating messages. > > Done. > > https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... > test/cctest/interpreter/generate-bytecode-expectations.cc:476: std::cerr << > "Reading " << input_filename << '\n'; > On 2016/02/25 13:12:38, oth wrote: > > This looks like left over debugging. > > Actually it's not, I thought it could give some feedback about the progress > while the tool processes all the golden files. If you think it's just noise, I > can remove it. Woops I saw you meant the one in DoRawJS. Yes, I think it's better to remove this and keep just the other.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/3331)
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/80001
https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:434: int DoRebaseline(const V8InitializationScope& platform, As discussed offline, let's try to merge these three functions to avoid duplicated code. https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:476: std::cerr << "Reading " << input_filename << '\n'; On 2016/02/25 13:23:56, ssanfilippo wrote: > On 2016/02/25 13:12:38, oth wrote: > > This looks like left over debugging. > > Actually it's not, I thought it could give some feedback about the progress > while the tool processes all the golden files. If you think it's just noise, I > can remove it. I think just remove this, the tool should be as silent as possible unless things go wrong (you could add a --verbose option though if you like).
https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:434: int DoRebaseline(const V8InitializationScope& platform, On 2016/02/25 14:12:28, rmcilroy wrote: > As discussed offline, let's try to merge these three functions to avoid > duplicated code. I'm working on it right now. https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... test/cctest/interpreter/generate-bytecode-expectations.cc:476: std::cerr << "Reading " << input_filename << '\n'; On 2016/02/25 14:12:28, rmcilroy wrote: > On 2016/02/25 13:23:56, ssanfilippo wrote: > > On 2016/02/25 13:12:38, oth wrote: > > > This looks like left over debugging. > > > > Actually it's not, I thought it could give some feedback about the progress > > while the tool processes all the golden files. If you think it's just noise, I > > can remove it. > > I think just remove this, the tool should be as silent as possible unless things > go wrong (you could add a --verbose option though if you like). Added a verbose flag. Done.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/120001
On 2016/02/25 14:23:50, ssanfilippo wrote: > https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... > File test/cctest/interpreter/generate-bytecode-expectations.cc (right): > > https://codereview.chromium.org/1737623002/diff/1/test/cctest/interpreter/gen... > test/cctest/interpreter/generate-bytecode-expectations.cc:434: int > DoRebaseline(const V8InitializationScope& platform, > On 2016/02/25 14:12:28, rmcilroy wrote: > > As discussed offline, let's try to merge these three functions to avoid > > duplicated code. > > I'm working on it right now. Done.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/140001
A couple of comments, but otherwise LGTM. https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:21: #define REPORT_ERROR(MESSAGE) (((std::cerr << "ERROR: ") << MESSAGE) << '\n') Don't make this a #define, use a helper function if required. https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:486: updated_options); Could you wrap all the code from 493-550 into a single function (which takes a filename which might be nullptr for stdout) and call it from both here and below?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/16018)
https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:21: #define REPORT_ERROR(MESSAGE) (((std::cerr << "ERROR: ") << MESSAGE) << '\n') On 2016/02/25 15:48:23, rmcilroy wrote: > Don't make this a #define, use a helper function if required. I used a macro instead of a function to enable the following syntax: ERROR("Foo is not a " << bar << " baz."); WDYT? https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:486: updated_options); On 2016/02/25 15:48:23, rmcilroy wrote: > Could you wrap all the code from 493-550 into a single function (which takes a > filename which might be nullptr for stdout) and call it from both here and > below? Done.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/180001
https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:21: #define REPORT_ERROR(MESSAGE) (((std::cerr << "ERROR: ") << MESSAGE) << '\n') On 2016/02/25 16:34:43, ssanfilippo wrote: > On 2016/02/25 15:48:23, rmcilroy wrote: > > Don't make this a #define, use a helper function if required. > > I used a macro instead of a function to enable the following syntax: > > ERROR("Foo is not a " << bar << " baz."); > > WDYT? How about just using v8/base/logging FATAL instead (that way you also wouldn't have to keep returning true/false everywhere and returning on errors since FATAL will just exit immediately with an error code) WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/200001
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Interpreter] Multiple input files for generate-bytecode-expectations. When operating in --rebaseline mode, each of the files will be updated. In --raw-js mode, all the expectations will be written to the same file. In default mode no more than one input file is accepted. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Multiple input files for generate-bytecode-expectations. When operating in --rebaseline mode, each of the files will be updated. In --raw-js mode, all the expectations will be written to the same file. In default mode no more than one input file is accepted. On POSIX systems, --rebaseline will autodiscover golden files when run from the project root and no input file is provided. BUG=v8:4280 LOG=N ==========
lgtm with a comment (also consider the previous comment about using FATAL. https://codereview.chromium.org/1737623002/diff/220001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/220001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:169: if (string[i] != suffix[j]) return false; you could use memcmp here.
On 2016/02/25 16:39:07, rmcilroy wrote: > https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... > File test/cctest/interpreter/generate-bytecode-expectations.cc (right): > > https://codereview.chromium.org/1737623002/diff/120001/test/cctest/interprete... > test/cctest/interpreter/generate-bytecode-expectations.cc:21: #define > REPORT_ERROR(MESSAGE) (((std::cerr << "ERROR: ") << MESSAGE) << '\n') > On 2016/02/25 16:34:43, ssanfilippo wrote: > > On 2016/02/25 15:48:23, rmcilroy wrote: > > > Don't make this a #define, use a helper function if required. > > > > I used a macro instead of a function to enable the following syntax: > > > > ERROR("Foo is not a " << bar << " baz."); > > > > WDYT? > > How about just using v8/base/logging FATAL instead (that way you also wouldn't > have to keep returning true/false everywhere and returning on errors since FATAL > will just exit immediately with an error code) WDYT? FATAL appears to generate a lot of additional noise, such as source location [1] and a backtrace [2]. I'd like to keep error messages as clean and readable as possible. [1] https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/loggin... [2] https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/loggin...
https://codereview.chromium.org/1737623002/diff/220001/test/cctest/interprete... File test/cctest/interpreter/generate-bytecode-expectations.cc (right): https://codereview.chromium.org/1737623002/diff/220001/test/cctest/interprete... test/cctest/interpreter/generate-bytecode-expectations.cc:169: if (string[i] != suffix[j]) return false; On 2016/02/26 08:56:16, rmcilroy wrote: > you could use memcmp here. Done.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/230002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/230002
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssanfilippo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1737623002/#ps230002 (title: "Remove unneeded special case.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737623002/230002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737623002/230002
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Multiple input files for generate-bytecode-expectations. When operating in --rebaseline mode, each of the files will be updated. In --raw-js mode, all the expectations will be written to the same file. In default mode no more than one input file is accepted. On POSIX systems, --rebaseline will autodiscover golden files when run from the project root and no input file is provided. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Multiple input files for generate-bytecode-expectations. When operating in --rebaseline mode, each of the files will be updated. In --raw-js mode, all the expectations will be written to the same file. In default mode no more than one input file is accepted. On POSIX systems, --rebaseline will autodiscover golden files when run from the project root and no input file is provided. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #16 (id:230002)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Multiple input files for generate-bytecode-expectations. When operating in --rebaseline mode, each of the files will be updated. In --raw-js mode, all the expectations will be written to the same file. In default mode no more than one input file is accepted. On POSIX systems, --rebaseline will autodiscover golden files when run from the project root and no input file is provided. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Multiple input files for generate-bytecode-expectations. When operating in --rebaseline mode, each of the files will be updated. In --raw-js mode, all the expectations will be written to the same file. In default mode no more than one input file is accepted. On POSIX systems, --rebaseline will autodiscover golden files when run from the project root and no input file is provided. BUG=v8:4280 LOG=N Committed: https://crrev.com/e039f63a4f809b940f67498b85749be31fd8b82a Cr-Commit-Position: refs/heads/master@{#34324} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/e039f63a4f809b940f67498b85749be31fd8b82a Cr-Commit-Position: refs/heads/master@{#34324} |
