[Courgette] Refactor: Add CourgetteFlow; improve courgette_tool.cc help text.

[Courgette] Refactor: Add CourgetteFlow; improve courgette_tool.cc help text.

High level flows in Courgette has messy error-handling code that's
hard to read. This CL adds CourgetteFlow to absorb commonly used data
and error-handling code, and improve readability of high-level logic.
Additional changes:
- Fix Courgette-disadj (debugging flow), which likely was broken by
  http://crrev.com/2793153003; Adjust() is called but AssemblyProgram
  did not annotate Labels.
- Add BasicBuffer interface, implemented by RegionBuffer and (existing)
  BufferedFileReader.
- PatchGeneratorX86_32::Transform(): Implemented TODO to instantiate
  "new" AssemblyProgram to reduce Courgette-gen peak memory.
- courgette_tools.cc: Make help text more uniform.

BUG=660980
Review-Url: https://codereview.chromium.org/2827103002
Cr-Commit-Position: refs/heads/master@{#467157}
Committed: https://chromium.googlesource.com/chromium/src/+/8b91a4c9f1d554685ee98468c96ee3c374b6633c

[huangs, 2017-04-19 18:22:47]

Description was changed from

==========
[Courgette] Refactoring: Add CourgetteFlow; improve courgette_tool.cc help text.

High level flows in Courgette has messy error-handling codes that makes
code hard to read. This CL adds CourgetteFlow to absorb commonly used
data and error-handling code, and improve readability of high-level
logic. Additional changes:
- Fix Courgette-disadj (debugging flow), which likely was broken by
  http://crrev.com/2793153003; Adjust() is called but AssemblyProgram
  did not annotate Labels.
- Add BasicBuffer interface, implemented by RegionBuffer and (existing)
  BufferedFileReader.
- courgette_tools.cc: Make help text more uniform.

BUG=660980
==========

to

==========
[Courgette] Refactoring: Add CourgetteFlow; improve courgette_tool.cc help text.

High level flows in Courgette has messy error-handling code that's
hard to read. This CL adds CourgetteFlow to absorb commonly used data
and error-handling code, and improve readability of high-level logic.
Additional changes:
- Fix Courgette-disadj (debugging flow), which likely was broken by
  http://crrev.com/2793153003; Adjust() is called but AssemblyProgram
  did not annotate Labels.
- Add BasicBuffer interface, implemented by RegionBuffer and (existing)
  BufferedFileReader.
- PatchGeneratorX86_32::Transform(): Implemented TODO to instantiate
  "new" AssemblyProgram to reduce Courgette-gen peak memory.
- courgette_tools.cc: Make help text more uniform.

BUG=660980
==========

[huangs, 2017-04-19 18:22:48]

huangs@chromium.org changed reviewers:
+ chrisha@chromium.org

[huangs, 2017-04-19 18:33:07]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[huangs, 2017-04-19 18:33:16]

PTAL. Thanks!

[commit-bot: I haz the power, 2017-04-19 18:34:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 19:44:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 19:44:27]

Dry run: This issue passed the CQ dry run.

[huangs, 2017-04-19 19:51:56]

Description was changed from

==========
[Courgette] Refactoring: Add CourgetteFlow; improve courgette_tool.cc help text.

High level flows in Courgette has messy error-handling code that's
hard to read. This CL adds CourgetteFlow to absorb commonly used data
and error-handling code, and improve readability of high-level logic.
Additional changes:
- Fix Courgette-disadj (debugging flow), which likely was broken by
  http://crrev.com/2793153003; Adjust() is called but AssemblyProgram
  did not annotate Labels.
- Add BasicBuffer interface, implemented by RegionBuffer and (existing)
  BufferedFileReader.
- PatchGeneratorX86_32::Transform(): Implemented TODO to instantiate
  "new" AssemblyProgram to reduce Courgette-gen peak memory.
- courgette_tools.cc: Make help text more uniform.

BUG=660980
==========

to

==========
[Courgette] Refactor: Add CourgetteFlow; improve courgette_tool.cc help text.

High level flows in Courgette has messy error-handling code that's
hard to read. This CL adds CourgetteFlow to absorb commonly used data
and error-handling code, and improve readability of high-level logic.
Additional changes:
- Fix Courgette-disadj (debugging flow), which likely was broken by
  http://crrev.com/2793153003; Adjust() is called but AssemblyProgram
  did not annotate Labels.
- Add BasicBuffer interface, implemented by RegionBuffer and (existing)
  BufferedFileReader.
- PatchGeneratorX86_32::Transform(): Implemented TODO to instantiate
  "new" AssemblyProgram to reduce Courgette-gen peak memory.
- courgette_tools.cc: Make help text more uniform.

BUG=660980
==========

[chrisha, 2017-04-24 18:15:50]

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
File courgette/courgette_flow.cc (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.cc:80: if (ok()) {
if (failed())
  return;

Then the rest of the logic doesn't need to be indented?

(Here and elsewhere)

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flow.h
File courgette/courgette_flow.h (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:56: enum Group { ONLY, OLD, NEW };
Could use comments? What is "ONLY"?

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:68: // Commands  that perform no-op on error. This
allows caller to concisely
remove extra space

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:73: void ReadSourceStreamSetFromBuffer(Group group,
const BasicBuffer& buffer);
In general, all of these functions could use comments describing the logic.

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:79: // If |opt_sources| is unspecified then
|data(which)->sources| is used.
|which|? Do you mean |group|?

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:105: //   if (!check(some_couregtte_function(param1,
...)))
courgette*

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_too...
File courgette/courgette_tool.cc (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_too...
courgette/courgette_tool.cc:167: WriteSinkToFile(&sink, output_file);
Does this still need to happen, even if failed() is true?

https://codereview.chromium.org/2827103002/diff/60001/courgette/encode_decode...
File courgette/encode_decode_unittest.cc (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/encode_decode...
courgette/encode_decode_unittest.cc:28: size_t expected_encoded_length) const;
Do you need to test the direct calls *and* the flow mechanism? Or is using the
flow mechanism sufficient?

https://codereview.chromium.org/2827103002/diff/60001/courgette/streams.h
File courgette/streams.h (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/streams.h#new...
courgette/streams.h:39: BasicBuffer() {}
Virtual destructor needed for interface classes.

[huangs, 2017-04-24 20:08:14]

Updated, also changing Fuzz test. PTAL. Thanks!

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
File courgette/courgette_flow.cc (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.cc:80: if (ok()) {
Initially I did that, then I got spooked by the bug

  if (failed())
    return     // Missing semicolon
  data(group)->program.reset();

which compiles, but fortunately got caught by the formatter! Anyway I prefer the
early-exit pattern, so changing back.

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flow.h
File courgette/courgette_flow.h (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:56: enum Group { ONLY, OLD, NEW };
On 2017/04/24 18:15:50, chrisha wrote:
> Could use comments? What is "ONLY"?

Done.

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:68: // Commands  that perform no-op on error. This
allows caller to concisely
On 2017/04/24 18:15:50, chrisha wrote:
> remove extra space

Done.

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:73: void ReadSourceStreamSetFromBuffer(Group group,
const BasicBuffer& buffer);
On 2017/04/24 18:15:50, chrisha wrote:
> In general, all of these functions could use comments describing the logic.

Done.

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:79: // If |opt_sources| is unspecified then
|data(which)->sources| is used.
On 2017/04/24 18:15:50, chrisha wrote:
> |which|? Do you mean |group|?

Done.

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_flo...
courgette/courgette_flow.h:105: //   if (!check(some_couregtte_function(param1,
...)))
On 2017/04/24 18:15:50, chrisha wrote:
> courgette*

Done.

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_too...
File courgette/courgette_tool.cc (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/courgette_too...
courgette/courgette_tool.cc:167: WriteSinkToFile(&sink, output_file);
On 2017/04/24 18:15:50, chrisha wrote:
> Does this still need to happen, even if failed() is true?

Problem() calls exit(1), so on failure this will not be called.

https://codereview.chromium.org/2827103002/diff/60001/courgette/encode_decode...
File courgette/encode_decode_unittest.cc (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/encode_decode...
courgette/encode_decode_unittest.cc:28: size_t expected_encoded_length) const;
On 2017/04/24 18:15:50, chrisha wrote:
> Do you need to test the direct calls *and* the flow mechanism? Or is using the
> flow mechanism sufficient?

Flow mechanism replaces the old stuff. So yeah it's better to remove duplicate.
Also updating encode_program_fuzz_unittests.cc (currently Fuzz fails though; fix
that later).

https://codereview.chromium.org/2827103002/diff/60001/courgette/streams.h
File courgette/streams.h (right):

https://codereview.chromium.org/2827103002/diff/60001/courgette/streams.h#new...
courgette/streams.h:39: BasicBuffer() {}
On 2017/04/24 18:15:50, chrisha wrote:
> Virtual destructor needed for interface classes.

Done. Also getting the 2 implementations to override.

[chrisha, 2017-04-25 20:58:37]

lgtm!

[huangs, 2017-04-25 21:00:13]

Thanks. Committing!

[huangs, 2017-04-25 21:00:28]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 21:01:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 22:21:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 22:21:59]

Dry run: This issue passed the CQ dry run.

[huangs, 2017-04-25 22:56:27]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2017-04-25 22:56:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 23:05:05]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493160987000330,
"parent_rev": "7fa45855fb213b42550245183ee37ce33d5e0902", "commit_rev":
"8b91a4c9f1d554685ee98468c96ee3c374b6633c"}

[commit-bot: I haz the power, 2017-04-25 23:05:12]

Description was changed from

==========
[Courgette] Refactor: Add CourgetteFlow; improve courgette_tool.cc help text.

High level flows in Courgette has messy error-handling code that's
hard to read. This CL adds CourgetteFlow to absorb commonly used data
and error-handling code, and improve readability of high-level logic.
Additional changes:
- Fix Courgette-disadj (debugging flow), which likely was broken by
  http://crrev.com/2793153003; Adjust() is called but AssemblyProgram
  did not annotate Labels.
- Add BasicBuffer interface, implemented by RegionBuffer and (existing)
  BufferedFileReader.
- PatchGeneratorX86_32::Transform(): Implemented TODO to instantiate
  "new" AssemblyProgram to reduce Courgette-gen peak memory.
- courgette_tools.cc: Make help text more uniform.

BUG=660980
==========

to

==========
[Courgette] Refactor: Add CourgetteFlow; improve courgette_tool.cc help text.

High level flows in Courgette has messy error-handling code that's
hard to read. This CL adds CourgetteFlow to absorb commonly used data
and error-handling code, and improve readability of high-level logic.
Additional changes:
- Fix Courgette-disadj (debugging flow), which likely was broken by
  http://crrev.com/2793153003; Adjust() is called but AssemblyProgram
  did not annotate Labels.
- Add BasicBuffer interface, implemented by RegionBuffer and (existing)
  BufferedFileReader.
- PatchGeneratorX86_32::Transform(): Implemented TODO to instantiate
  "new" AssemblyProgram to reduce Courgette-gen peak memory.
- courgette_tools.cc: Make help text more uniform.

BUG=660980

Review-Url: https://codereview.chromium.org/2827103002
Cr-Commit-Position: refs/heads/master@{#467157}
Committed:
https://chromium.googlesource.com/chromium/src/+/8b91a4c9f1d554685ee98468c96e...
==========

[commit-bot: I haz the power, 2017-04-25 23:05:14]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8b91a4c9f1d554685ee98468c96e...