Hi Dean This is a first good step. Please commit you changes. The changes I suggested can be added later. LGTM
Regards, Lars On Thu, Sep 11, 2008 at 4:15 PM, Dean McNamee <[EMAIL PROTECTED]> wrote: > On Thu, Sep 11, 2008 at 4:11 PM, <[EMAIL PROTECTED]> wrote: >> Hi Dean >> >> Great first step to move all flags to one file. >> However, it would be perfect if you could eliminate the flags.defs. >> >> Otherwise, LGTM, >> Lars >> >> >> http://codereview.chromium.org/1935/diff/53/271 >> File src/flags.defs (right): >> >> http://codereview.chromium.org/1935/diff/53/271#newcode1 >> Line 1: // Copyright 2008 the V8 project authors. All rights reserved. >> I understand the reason for the state based include but we should reduce >> flags to be in release or in release+debug. >> Since most flags is compiled away in release mode, I think this is OK. >> If you make this change flags can be defined in ONE giant macro. >> This will also eliminate the need for flags.def. > > Please see v8-counters.h to why this won't / doesn't work well. > > I agree it would be nice to have less "modes", but maybe that's > something we could refactor later? I wanted to make this change, > leaving the flags system working identically to how it works now. > Changing both at the same time is too many moving pieces, this was a > pain enough as is... > >> >> http://codereview.chromium.org/1935/diff/53/272 >> File src/flags.h (right): >> >> http://codereview.chromium.org/1935/diff/53/272#newcode35 >> Line 35: #define FLAG_MODE_DECLARE >> I don't like the defs file extension. Please use .h as extension. After >> all, it is a C++ header file. > > This file won't lint, and it's not really a header in the normal > sense. Making it a .h will break the build unless we make a lint > exception. It's up to you I guess. > >> >> http://codereview.chromium.org/1935 >> > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
