[v8-dev] Re: Implement --omit, --raw_[context_]file=... for mksnapshot tool. (issue 249283002)
https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h#newcode660
src/flag-definitions.h:660: DEFINE_bool(omit, false, Omit raw snapshot
bytes in generated code.)
These new flags don't make sense outside of mksnapshot, can we find a
way to restrict them to that? Having them e.g. in d8 is a bit confusing.
I know this problem has been there before, but now we have 4 confusing
flags instead of 1.
https://codereview.chromium.org/249283002/diff/20001/src/mksnapshot.cc
File src/mksnapshot.cc (right):
https://codereview.chromium.org/249283002/diff/20001/src/mksnapshot.cc#newcode75
src/mksnapshot.cc:75: ~SnapshotWriter() {
Here and below: We normally use 1 blank line between function
definitions in classes and 2 blank lines between out-of-line function
definitions. We sometimes deviate from these rules for single one-line
getter/setter functions.
https://codereview.chromium.org/249283002/diff/20001/src/mksnapshot.cc#newcode116
src/mksnapshot.cc:116: void WriteData(const char* prefix, const
i::Listchar source_data,
Here and at other places: If parameters don't fit on a line, we usually
have a single parameter per line.
https://codereview.chromium.org/249283002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Implement --omit, --raw_[context_]file=... for mksnapshot tool. (issue 249283002)
On 2014/04/24 06:59:16, Sven Panne wrote:
https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h#newcode660
src/flag-definitions.h:660: DEFINE_bool(omit, false, Omit raw snapshot
bytes
in
generated code.)
These new flags don't make sense outside of mksnapshot, can we find a way
to
restrict them to that? Having them e.g. in d8 is a bit confusing. I know
this
problem has been there before, but now we have 4 confusing flags instead
of 1.
https://codereview.chromium.org/249283002/diff/20001/src/mksnapshot.cc
File src/mksnapshot.cc (right):
https://codereview.chromium.org/249283002/diff/20001/src/mksnapshot.cc#newcode75
src/mksnapshot.cc:75: ~SnapshotWriter() {
Here and below: We normally use 1 blank line between function definitions
in
classes and 2 blank lines between out-of-line function definitions. We
sometimes
deviate from these rules for single one-line getter/setter functions.
https://codereview.chromium.org/249283002/diff/20001/src/mksnapshot.cc#newcode116
src/mksnapshot.cc:116: void WriteData(const char* prefix, const
i::Listchar
source_data,
Here and at other places: If parameters don't fit on a line, we usually
have a
single parameter per line.
I'm not really a huge fan of those unwritten coding style rules. The V8
style
guide says that the code should follow the Google C++ style guide :-/
https://codereview.chromium.org/249283002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Implement --omit, --raw_[context_]file=... for mksnapshot tool. (issue 249283002)
PTAL. Not sure what to do abount the flags thing, though.
https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h#newcode660
src/flag-definitions.h:660: DEFINE_bool(omit, false, Omit raw snapshot
bytes in generated code.)
On 2014/04/24 06:59:17, Sven Panne wrote:
These new flags don't make sense outside of mksnapshot, can we find a
way to
restrict them to that? Having them e.g. in d8 is a bit confusing. I
know this
problem has been there before, but now we have 4 confusing flags
instead of 1.
I looked at this, thinking it should be really easy to just move those
flags to mksnapshot.cc which contains the main method. Except... the way
the current flags.h/.cc infrastructure is built, it relies on repeatedly
including flag-definitions.h with various defines set to 'extract'
different data out of the macros there. There appears to be no way to
modify the data structures used by flags.h/.cc other than
flag-definitions.h; at least I didn't find any.
Honestly, I'm not super fond of this design, but I can't see any fix
other than refactoring the whole flags thing.
https://codereview.chromium.org/249283002/diff/20001/src/mksnapshot.cc
File src/mksnapshot.cc (right):
https://codereview.chromium.org/249283002/diff/20001/src/mksnapshot.cc#newcode75
src/mksnapshot.cc:75: ~SnapshotWriter() {
On 2014/04/24 06:59:17, Sven Panne wrote:
Here and below: We normally use 1 blank line between function
definitions in
classes and 2 blank lines between out-of-line function definitions. We
sometimes
deviate from these rules for single one-line getter/setter functions.
Done.
https://codereview.chromium.org/249283002/diff/20001/src/mksnapshot.cc#newcode116
src/mksnapshot.cc:116: void WriteData(const char* prefix, const
i::Listchar source_data,
On 2014/04/24 06:59:17, Sven Panne wrote:
Here and at other places: If parameters don't fit on a line, we
usually have a
single parameter per line.
Done.
https://codereview.chromium.org/249283002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Implement --omit, --raw_[context_]file=... for mksnapshot tool. (issue 249283002)
LGTM with a nit https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h#newcode660 src/flag-definitions.h:660: DEFINE_bool(omit, false, Omit raw snapshot bytes in generated code.) On 2014/04/24 11:07:59, vogelheim wrote: [...] Honestly, I'm not super fond of this design, but I can't see any fix other than refactoring the whole flags thing. I suspected that... :-/ OK, then just add (mksnapshot only) to the descriptions. https://codereview.chromium.org/249283002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups v8-dev group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Implement --omit, --raw_[context_]file=... for mksnapshot tool. (issue 249283002)
On 2014/04/24 11:12:43, Sven Panne wrote: LGTM with a nit https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/249283002/diff/20001/src/flag-definitions.h#newcode660 src/flag-definitions.h:660: DEFINE_bool(omit, false, Omit raw snapshot bytes in generated code.) On 2014/04/24 11:07:59, vogelheim wrote: [...] Honestly, I'm not super fond of this design, but I can't see any fix other than refactoring the whole flags thing. I suspected that... :-/ OK, then just add (mksnapshot only) to the descriptions. Done. https://codereview.chromium.org/249283002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups v8-dev group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Implement --omit, --raw_[context_]file=... for mksnapshot tool. (issue 249283002)
Committed patchset #3 manually as r20941 (presubmit successful). https://codereview.chromium.org/249283002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups v8-dev group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Implement --omit, --raw_[context_]file=... for mksnapshot tool. (issue 249283002)
lgtm I guess make quickcheck still passes with this? https://codereview.chromium.org/249283002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups v8-dev group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Implement --omit, --raw_[context_]file=... for mksnapshot tool. (issue 249283002)
On Wed, Apr 23, 2014 at 5:23 PM, joc...@chromium.org wrote: lgtm I guess make quickcheck still passes with this? Indeed. -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups v8-dev group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
