Mojo cpp bindings: report the reason of validation failure.

Mojo cpp bindings: report the reason of validation failure.

Validation errors are written to stderr and also used in validation tests.
This CL also changes MessageHeaderValidator to use the same struct header
validation code as other structs.

BUG=None
TEST=validation_unittest.cc

[yzshen1, 2014-06-03 21:00:01]

Hi, Darin.

Would you please take a look? Thanks!

[darin (slow to review), 2014-06-03 22:47:49]

Nice, LGTM!

I wonder if we shouldn't do more to enable these code paths to be optimized away
in release builds.

[yzshen1, 2014-06-04 04:28:56]

On 2014/06/03 22:47:49, darin wrote:
> Nice, LGTM!
> 
> I wonder if we shouldn't do more to enable these code paths to be optimized
away
> in release builds.

I think performance-wise it shouldn't be a problem. ReportValidationError() is
only run when validation fails. Since almost all messages should pass
validation, this code is not on the critical path. Besides, it only does an
enum-to-string conversion, a fprintf() call and a NULL check of a pointer (for
ValidationErrorObserverForTesting), the cost should be pretty small.

Are you concerned about excessive output to stderr in release builds? I think it
shouldn't be too much, but I could turn it off if you think that is better.

Or maybe I haven't understood your idea. Please let me know. Thanks!

[darin (slow to review), 2014-06-04 04:37:40]

Good point. No worries!
-Darin


On Tue, Jun 3, 2014 at 9:28 PM, <yzshen@chromium.org> wrote:

> On 2014/06/03 22:47:49, darin wrote:
>
>> Nice, LGTM!
>>
>
>  I wonder if we shouldn't do more to enable these code paths to be
>> optimized
>>
> away
>
>> in release builds.
>>
>
> I think performance-wise it shouldn't be a problem.
> ReportValidationError() is
> only run when validation fails. Since almost all messages should pass
> validation, this code is not on the critical path. Besides, it only does an
> enum-to-string conversion, a fprintf() call and a NULL check of a pointer
> (for
> ValidationErrorObserverForTesting), the cost should be pretty small.
>
> Are you concerned about excessive output to stderr in release builds? I
> think it
> shouldn't be too much, but I could turn it off if you think that is better.
>
> Or maybe I haven't understood your idea. Please let me know. Thanks!
>
> https://codereview.chromium.org/307353009/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2014-06-04 04:48:56]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-06-04 04:50:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/307353009/1

[commit-bot: I haz the power, 2014-06-04 10:52:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-04 10:58:41]

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

[commit-bot: I haz the power, 2014-06-04 10:58:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)