New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 867823 link

Starred by 1 user
Status: Fixed
Owner:
█ vabr@chromium.org
hobby only
Closed: Aug 30
Cc:
pkasting@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

_CheckUniquePtr presubmit does not handle well custom deleters

Project Member Reported by mastiz@chromium.org, Jul 26 
The following code currently issues a presubmit error:
auto p = std::unique_ptr<T, D>(new T(), D());

...suggesting that std::make_unique should be used, but std::make_unique doesn't support deleters, and neither does base::WrapUnique().

Specifically, I'd trying to use base::OnTaskRunnerDeleter.

Perhaps WrapUnique() should be extended, but that seems orthogonal and a longer shot (absl also doesn't support that).

Comment 1 by vabr@chromium.org, Jul 26 

Thanks for the report.
I agree that this is a false positive, because make_unique cannot substitute this use.

I only have contributed to the presubmit check after I was bitten by some if its earlier errors, and the code is rather simple. If you feel like tweaking the regexp a little (and adding a test in PRESUBMIT_test.py), then feel free to go ahead.

I'm happy to have a look, but am on vacation until mid-August, so cannot help you until then.

Comment 2 by pkasting@chromium.org, Jul 26 

Same story :)

Comment 3 by vabr@chromium.org, Aug 28

Cc: -vabr@chromium.org
Owner: vabr@chromium.org
Status: Started (was: Available)
Back from vacation, https://crrev.com/c/1193945
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 30 

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

commit b7fadb6925ab81a27cf6718e78759cdb1a9cd1cb
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Aug 30 06:39:53 2018

Allow std::unique_ptr<T, D>(...) in presubmit

The _CheckUniquePtr presubmit check has the following false-positive:
it flags
  auto p = std::unique_ptr<T, D>(new T(), D());
as needing a rewrite using std::make_unique.

However, there seems to be no way to pass the second template argument
to std::make_unique, so cases like this need to be exempt from the
presubmit check.

Note that the check still needs to capture cases with

count the nesting of the angle brackets. Hence, this check cannot be
captured by a regular expression and needs just a counting loop.

std: :unique_ptr<T<U, V>>, so when looking for the comma, one has to
Bug:  867823 
Change-Id: Ie21067fa83aae17dd6701b0f69b902605c797e67
Reviewed-on: https://chromium-review.googlesource.com/1193945
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587463}
[modify] https://crrev.com/b7fadb6925ab81a27cf6718e78759cdb1a9cd1cb/PRESUBMIT.py
[modify] https://crrev.com/b7fadb6925ab81a27cf6718e78759cdb1a9cd1cb/PRESUBMIT_test.py

Comment 5 by vabr@chromium.org, Aug 30

Status: Fixed (was: Started)
r587463 says: Have fun using custom deleters without presubmit alarms! :)
►

Sign in to add a comment