Initial drop of the DFA-based Ragel-based validator.

Initial drop of the DFA-based Ragel-based validator.

The previous history is at: https://github.com/khimru/ncval_ragel
This addition matches commit: abf4e6c7ffe2ec3b59acb589795a3c574a69f6da
The code style must be cleaned up later.
The integration with build systems (scons, gyp) must be done later.

BUG=none
TEST=see inside

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=7787

[khim, 2012-02-13 16:47:41]

LGTM

Cleanups will be in follow-up commits...

[Mark Seaborn, 2012-02-13 17:09:22]

Please can you drop the test binary, nacl_irt_x86_64.nexe.  It just bloats the
SVN repository and is available as part of the normal NaCl build.

https://chromiumcodereview.appspot.com/9386010/diff/1/src/trusted/validator_r...
File src/trusted/validator_ragel/gen-decoder.C (right):

https://chromiumcodereview.appspot.com/9386010/diff/1/src/trusted/validator_r...
src/trusted/validator_ragel/gen-decoder.C:1: /*
Why does this file have a capital .C extension?  This seems non-standard.

[pasko-google - do not use, 2012-02-13 17:13:47]

On 2012/02/13 17:09:22, Mark Seaborn wrote:
> Please can you drop the test binary, nacl_irt_x86_64.nexe.  It just bloats the
> SVN repository and is available as part of the normal NaCl build.

going to remove soon, it is needed for tests now

>
https://chromiumcodereview.appspot.com/9386010/diff/1/src/trusted/validator_r...
> File src/trusted/validator_ragel/gen-decoder.C (right):
> 
>
https://chromiumcodereview.appspot.com/9386010/diff/1/src/trusted/validator_r...
> src/trusted/validator_ragel/gen-decoder.C:1: /*
> Why does this file have a capital .C extension?  This seems non-standard.

it is recognized by GNU make as a valid extension for C++, but it is not
recommended, I agree. Also, it does not fit our style in native_client, hence,
will be renamed. Actually, the name "gen-decoder" is awkward and historical, a
better name is "gen-dfa" since it is used to generate code for both decoder and
validator.

[Mark Seaborn, 2012-02-13 17:30:13]

On 13 February 2012 09:13, <pasko@google.com> wrote:

> On 2012/02/13 17:09:22, Mark Seaborn wrote:
>
>> Please can you drop the test binary, nacl_irt_x86_64.nexe.  It just
>> bloats the
>> SVN repository and is available as part of the normal NaCl build.
>>
>
> going to remove soon, it is needed for tests now
>

Gah.  You actually committed it, so it has permanently increased the size
of the Git repo.


Call me crazy, but I would have thought the purpose of the first review for
this code would be to actually review it.  As it is, this was just a
rubber-stamp.  You've bypassed the normal requirement that code in the SVN
repo be reviewed, which does not seem like a good start for the most
complicated component in the TCB.  Even if you have reviews for future
changes, the status of any line of code is going to have to be "assume by
default that it was unreviewed".

This was always going to be an issue for a substantial piece of code
developed outside of NaCl SVN, but I rather expected you would share your
plans first, provide some forewarning of what to expect, etc.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[khim, 2012-02-13 18:53:55]

On Mon, Feb 13, 2012 at 9:30 PM, Mark Seaborn <mseaborn@chromium.org> wrote:

> On 13 February 2012 09:13, <pasko@google.com> wrote:
>
>> On 2012/02/13 17:09:22, Mark Seaborn wrote:
>>
>>> Please can you drop the test binary, nacl_irt_x86_64.nexe.  It just
>>> bloats the
>>> SVN repository and is available as part of the normal NaCl build.
>>>
>>
>> going to remove soon, it is needed for tests now
>>
>
> Gah.  You actually committed it, so it has permanently increased the size
> of the Git repo.
>
>
> Call me crazy, but I would have thought the purpose of the first review
> for this code would be to actually review it.
>

It was reviewed. I've compared it with the code in git repo and checked
that it can be pulled and smoke test pass.


> As it is, this was just a rubber-stamp.  You've bypassed the normal
> requirement that code in the SVN repo be reviewed, which does not seem like
> a good start for the most complicated component in the TCB.  Even if you
> have reviews for future changes, the status of any line of code is going to
> have to be "assume by default that it was unreviewed".
>

That actually matches the status of all major components in the repo
including validator: they all were created in CL#191 named "load 1099 into
trunk". But I agree that it's unfortunate consequence.


> This was always going to be an issue for a substantial piece of code
> developed outside of NaCl SVN, but I rather expected you would share your
> plans first, provide some forewarning of what to expect, etc.
>

We HAVE discussed our plans, but apparently these discussions were not
detailed enough. Exactly because it's core component and because it needs
thorough review we wanted to have it committed to SVN now instead of later:
traditional review workflow assumes only one person changes the files and
usually others don't even compile and run them while this process is
underway (theretically they could but very few actually do). This works
fine for small CLs, it does not work as well for hundred lines CL and for
changes which include thousand lines or more it just creates unnecessary
tension and stalls the process.

We've talked about this on Core Runtime meeting and Bennet offered a
solution: we can create subdirectory named "unreviewed" and move all files
to it. Then we can collect various opinions and change files
to accommodate requested changes.

As for sizes... we plan to commit generated files in SVN (like it's done
with current validator) to make sure everything can be built on system
without ragel (in particular on Mac and Windows). Currently these files are
13MB in size uncompressed (and about 600KB compressed) which is about the
same size as IRT and, unlike IRT, they will be changed often.

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.