New issue
Advanced search Search tips

Issue 596278 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Tautologous DCHECK in JSONParser implementation

Project Member Reported by w...@chromium.org, Mar 19 2016

Issue description

Replacing DCHECK_OP()'s implementation with DCHECK((val1) op (val2)) in a debug build gives compilation error in base::JSONParser:

https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_parser.cc&sq=package:chromium&l=312
  void JSONParser::StringBuilder::Append(const char& c) {
    DCHECK_GE(c, 0);
->  DCHECK_LT(c, 128);
    ...

Since |c| is unsigned its range is -128 to 127; it seems that there should either be just the DCHECK_GE, or for better readability, just a DCHECK_LT(static_cast<unsigned char>(c), ...)?
 

Comment 1 by rsesek@chromium.org, Mar 25 2016

Status: Started (was: Untriaged)
The original intent was I think your latter suggestion (treating it as a unichar). https://codereview.chromium.org/1834933003/
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19c6150642d7ab6a5a455b36b85efd6715028425

commit 19c6150642d7ab6a5a455b36b85efd6715028425
Author: rsesek <rsesek@chromium.org>
Date: Fri Mar 25 18:26:03 2016

Fix tautologous DCHECK in base::internal::JSONParser.

BUG= 596278 
R=mark@chromium.org

Review URL: https://codereview.chromium.org/1834933003

Cr-Commit-Position: refs/heads/master@{#383316}

[modify] https://crrev.com/19c6150642d7ab6a5a455b36b85efd6715028425/base/json/json_parser.cc

Comment 3 by rsesek@chromium.org, Mar 25 2016

Status: Fixed (was: Started)

Sign in to add a comment