Use universal references in templated analysis passthrough functions.

Use universal references in templated analysis passthrough functions.

The ANALYSIS_ASSUME_TRUE macros must support lvalues and rvalues without
copying. Currently we are taking arguments by-value; this does not work
for things like null checks on unique_ptr (DCHECK(my_unique_ptr)).
Taking a non-const reference doesn't work either, as one cannot take
a reference to an rvalue parameter. Multiple definitions for by-val
and by-ref don't work, because the choice between TVal and TVal&
is too ambiguous for the compiler.

Universal references work nicely in this case because they allow lvalue and rvalue parameters to be taken without making copies or the need for adding overloads.

Also added ANALYSIS_ASSUME_TRUE to DCHECK_OP implementation bodies.
This prevents the static analyzer from proceeding if a comparison-
based DCHECK fails, which is a static assertion that the asserted
condition will always hold.


R=danakj@chromium.org,wez@chromium.org
BUG=687243
Review-Url: https://codereview.chromium.org/2692853008
Cr-Commit-Position: refs/heads/master@{#452351}
Committed: https://chromium.googlesource.com/chromium/src/+/549cad5d1141989cc3cac9ae7161984b40ae5607

[danakj, 2017-02-14 20:09:17]

https://codereview.chromium.org/2692853008/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2692853008/diff/1/base/logging.h#newcode301
base/logging.h:301: // An rvalue parameter is used because it allows both lvalue
and rvalue forms of
This isn't an rvalue parameter actually, because TVal is a templated type. It's
a universal reference and can bind to an lvalue ref or rvalue ref.

https://codereview.chromium.org/2692853008/diff/1/base/logging.h#newcode639
base/logging.h:639: if (ANALYZER_ASSUME_TRUE(v1 op v2))                         
            \
I think this is a separate CL. Can you split this?

[Kevin M, 2017-02-14 21:04:40]

Description was changed from

==========
Add rvalue support to analysis passthrough functions.

The ANALYSIS_ASSUME_TRUE macros must support lvalues and rvalues without
copying. Currently we are taking arguments by-value; this does not work
for things like null checks on unique_ptr (DCHECK(my_unique_ptr)).
Taking a non-const reference doesn't work either, as one cannot take
a reference to an rvalue parameter. Multiple definitions for by-val
and by-ref don't work, because the choice between TVal and TVal&
is too ambiguous for the compiler.

Rvalues work best in this case because they allow lvalue and rvalue
parameters to be taken without making copies.

Also added ANALYSIS_ASSUME_TRUE to DCHECK_OP implementation bodies.
This prevents the static analyzer from proceeding if a comparison-
based DCHECK fails, which is a static assertion that the asserted
condition will always hold.


R=danakj@chromium.org,wez@chromium.org
BUG=
==========

to

==========
Use universal references in templated analysis passthrough functions.

The ANALYSIS_ASSUME_TRUE macros must support lvalues and rvalues without
copying. Currently we are taking arguments by-value; this does not work
for things like null checks on unique_ptr (DCHECK(my_unique_ptr)).
Taking a non-const reference doesn't work either, as one cannot take
a reference to an rvalue parameter. Multiple definitions for by-val
and by-ref don't work, because the choice between TVal and TVal&
is too ambiguous for the compiler.

Universal references work nicely in this case because they allow lvalue and
rvalue parameters to be taken without making copies or the need for adding
overloads.

Also added ANALYSIS_ASSUME_TRUE to DCHECK_OP implementation bodies.
This prevents the static analyzer from proceeding if a comparison-
based DCHECK fails, which is a static assertion that the asserted
condition will always hold.


R=danakj@chromium.org,wez@chromium.org
BUG=687243
==========

[Kevin M, 2017-02-14 21:05:09]

See https://codereview.chromium.org/2691243004 for the forked DCHECK_OP CL.

https://codereview.chromium.org/2692853008/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2692853008/diff/1/base/logging.h#newcode301
base/logging.h:301: // An rvalue parameter is used because it allows both lvalue
and rvalue forms of
On 2017/02/14 20:09:16, danakj wrote:
> This isn't an rvalue parameter actually, because TVal is a templated type.
It's
> a universal reference and can bind to an lvalue ref or rvalue ref.

Updated the comments.

https://codereview.chromium.org/2692853008/diff/1/base/logging.h#newcode639
base/logging.h:639: if (ANALYZER_ASSUME_TRUE(v1 op v2))                         
            \
On 2017/02/14 20:09:16, danakj wrote:
> I think this is a separate CL. Can you split this?

Done.

[danakj, 2017-02-14 21:10:10]

Hm, this kept both parts tho still, maybe its not ready yet? The description has
both parts in it still too.

https://codereview.chromium.org/2692853008/diff/20001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2692853008/diff/20001/base/logging.h#newcode304
base/logging.h:304: inline constexpr TVal AnalysisAssumeTrue(TVal arg) {
dropped the && here

[Kevin M, 2017-02-14 21:57:50]

Ah, I guess I misunderstood what you meant.

Removing the '&&' is incompatible with move-only data types like unique_ptr, as
it tries to copy the parameter by value which is unsupported.

Here's an example of the error:
../../mojo/public/cpp/bindings/lib/interface_ptr_state.h:178:14: error: call to
deleted constructor of 'std::unique_ptr<mojo::InterfaceEndpointClient,
std::default_delete<mojo::InterfaceEndpointClient> >'
      DCHECK(endpoint_client_);
             ^~~~~~~~~~~~~~~~
../../base/logging.h:737:57: note: expanded from macro 'DCHECK'
  LAZY_STREAM(LOG_STREAM(DCHECK), !ANALYZER_ASSUME_TRUE(condition)) \
                                                        ^~~~~~~~~
...

[danakj, 2017-02-14 21:59:22]

On Tue, Feb 14, 2017 at 4:57 PM, <kmarshall@chromium.org> wrote:

> Ah, I guess I misunderstood what you meant.
>
> Removing the '&&' is incompatible with move-only data types like
> unique_ptr, as
> it tries to copy the parameter by value which is unsupported.
>

Right I meant that your comment called it an rvalue but its a templated
type so its not an rvalue its a universal ref. The code was fine.


>
> Here's an example of the error:
> ../../mojo/public/cpp/bindings/lib/interface_ptr_state.h:178:14: error:
> call to
> deleted constructor of 'std::unique_ptr<mojo::InterfaceEndpointClient,
> std::default_delete<mojo::InterfaceEndpointClient> >'
> DCHECK(endpoint_client_);
> ^~~~~~~~~~~~~~~~
> ../../base/logging.h:737:57: note: expanded from macro 'DCHECK'
> LAZY_STREAM(LOG_STREAM(DCHECK), !ANALYZER_ASSUME_TRUE(condition)) \
> ^~~~~~~~~
> ...
>
> https://codereview.chromium.org/2692853008/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kevin M, 2017-02-14 22:07:48]

OK, done. :)

https://codereview.chromium.org/2692853008/diff/20001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2692853008/diff/20001/base/logging.h#newcode304
base/logging.h:304: inline constexpr TVal AnalysisAssumeTrue(TVal arg) {
On 2017/02/14 21:10:10, danakj wrote:
> dropped the && here

Done.

[danakj, 2017-02-14 22:09:39]

https://codereview.chromium.org/2692853008/diff/40001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2692853008/diff/40001/base/logging.h#newcode639
base/logging.h:639: if (ANALYZER_ASSUME_TRUE(v1 op v2))                         
            \
This is the other CL, it should be removed from this diff?

[Kevin M, 2017-02-14 22:29:17]

https://codereview.chromium.org/2692853008/diff/40001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2692853008/diff/40001/base/logging.h#newcode639
base/logging.h:639: if (ANALYZER_ASSUME_TRUE(v1 op v2))                         
            \
On 2017/02/14 22:09:39, danakj wrote:
> This is the other CL, it should be removed from this diff?

Removed

[danakj, 2017-02-14 22:39:04]

LGTM

[danakj, 2017-02-14 22:39:32]

https://codereview.chromium.org/2692853008/diff/80001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2692853008/diff/80001/base/logging.h#newcode12
base/logging.h:12: #include <memory>
forward is in <utility> tho

[Kevin M, 2017-02-14 22:41:01]

https://codereview.chromium.org/2692853008/diff/80001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2692853008/diff/80001/base/logging.h#newcode12
base/logging.h:12: #include <memory>
On 2017/02/14 22:39:31, danakj wrote:
> forward is in <utility> tho

Done.

[Kevin M, 2017-02-14 22:41:04]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2017-02-14 22:41:05]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2692853008/#ps100001
(title: "memory => utility for std::forward")

[commit-bot: I haz the power, 2017-02-14 22:42:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 00:40:28]

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

[commit-bot: I haz the power, 2017-02-15 00:40:29]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Kevin M, 2017-02-21 19:26:19]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-02-21 19:27:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 21:31:22]

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

[commit-bot: I haz the power, 2017-02-21 21:31:23]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Kevin M, 2017-02-22 23:10:26]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-02-22 23:11:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 02:30:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487805026694490,
"parent_rev": "2de1415949d2cb3f4e186bc94eddcefd77dd4635", "commit_rev":
"549cad5d1141989cc3cac9ae7161984b40ae5607"}

[commit-bot: I haz the power, 2017-02-23 02:31:17]

Description was changed from

==========
Use universal references in templated analysis passthrough functions.

The ANALYSIS_ASSUME_TRUE macros must support lvalues and rvalues without
copying. Currently we are taking arguments by-value; this does not work
for things like null checks on unique_ptr (DCHECK(my_unique_ptr)).
Taking a non-const reference doesn't work either, as one cannot take
a reference to an rvalue parameter. Multiple definitions for by-val
and by-ref don't work, because the choice between TVal and TVal&
is too ambiguous for the compiler.

Universal references work nicely in this case because they allow lvalue and
rvalue parameters to be taken without making copies or the need for adding
overloads.

Also added ANALYSIS_ASSUME_TRUE to DCHECK_OP implementation bodies.
This prevents the static analyzer from proceeding if a comparison-
based DCHECK fails, which is a static assertion that the asserted
condition will always hold.


R=danakj@chromium.org,wez@chromium.org
BUG=687243
==========

to

==========
Use universal references in templated analysis passthrough functions.

The ANALYSIS_ASSUME_TRUE macros must support lvalues and rvalues without
copying. Currently we are taking arguments by-value; this does not work
for things like null checks on unique_ptr (DCHECK(my_unique_ptr)).
Taking a non-const reference doesn't work either, as one cannot take
a reference to an rvalue parameter. Multiple definitions for by-val
and by-ref don't work, because the choice between TVal and TVal&
is too ambiguous for the compiler.

Universal references work nicely in this case because they allow lvalue and
rvalue parameters to be taken without making copies or the need for adding
overloads.

Also added ANALYSIS_ASSUME_TRUE to DCHECK_OP implementation bodies.
This prevents the static analyzer from proceeding if a comparison-
based DCHECK fails, which is a static assertion that the asserted
condition will always hold.


R=danakj@chromium.org,wez@chromium.org
BUG=687243

Review-Url: https://codereview.chromium.org/2692853008
Cr-Commit-Position: refs/heads/master@{#452351}
Committed:
https://chromium.googlesource.com/chromium/src/+/549cad5d1141989cc3cac9ae7161...
==========

[commit-bot: I haz the power, 2017-02-23 02:31:18]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/549cad5d1141989cc3cac9ae7161...