Get more consistent decisions if a statement is constant in templates

Get more consistent decisions if a statement is constant in templates

The tool was saying some template-dependent values were both const
and not const because when the template parameters meet some criteria
it collapses them out of the AST. If we always check the entire
subtree of expressions to verify they are evaluatable we get more
consistent results.

Also don't try to turn this_type_of_name into kThisTypeOfName, as
the author explicitly wrote it that way and we end up with the ever
fun kThis_type_of_name instead.

R=dcheng@chromium.org
BUG=677285
Review-Url: https://codereview.chromium.org/2616213003
Cr-Commit-Position: refs/heads/master@{#442262}
Committed: https://chromium.googlesource.com/chromium/src/+/b9a1485ee7f50c3980f8055d8f64ea4d30220720

[danakj, 2017-01-07 00:38:34]

Description was changed from

==========
Get more consistent decisions if a statement is constant in templates

The tool was saying some template-dependent values were both const
and not const because when the template parameters meet some criteria
it collapses them out of the AST. If we always check the entire
subtree of expressions to verify they are evaluatable we get more
consistent results.

R=dcheng@chromium.org
BUG=677285
==========

to

==========
Get more consistent decisions if a statement is constant in templates

The tool was saying some template-dependent values were both const
and not const because when the template parameters meet some criteria
it collapses them out of the AST. If we always check the entire
subtree of expressions to verify they are evaluatable we get more
consistent results.

Also don't try to turn this_type_of_name into kThisTypeOfName, as
the other explicitly wrote it that way and we end up with the ever
fun kThis_type_of_name instead.

R=dcheng@chromium.org
BUG=677285
==========

[dcheng, 2017-01-07 00:45:49]

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:374: // If the
statement is not an expression then it's a constant.
What would it be that makes it a constant?

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/tests/constants-expected.cc (right):

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/tests/constants-expected.cc:38: const int
should_be_renamed_to_a_const = 9 - 2;
maybe the prefix should be should_not_ now?

[danakj, 2017-01-07 00:48:00]

PTAL

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:374: // If the
statement is not an expression then it's a constant.
On 2017/01/07 00:45:49, dcheng wrote:
> What would it be that makes it a constant?

switch, if, return, language stuff. Not things that change at runtime.

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/tests/constants-expected.cc (right):

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/tests/constants-expected.cc:38: const int
should_be_renamed_to_a_const = 9 - 2;
On 2017/01/07 00:45:49, dcheng wrote:
> maybe the prefix should be should_not_ now?

Done.

[danakj, 2017-01-07 00:48:07]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 00:48:33]

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

[dcheng, 2017-01-07 00:50:18]

LGTM with comment addressed

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:374: // If the
statement is not an expression then it's a constant.
On 2017/01/07 00:48:00, danakj_OOO_and_slow wrote:
> On 2017/01/07 00:45:49, dcheng wrote:
> > What would it be that makes it a constant?
> 
> switch, if, return, language stuff. Not things that change at runtime.

I think we always pass in an Expr though, no? So this should just take an Expr,
and we can remove this if (it doesn't make sense for any of those to be
initializers, AFAIK)

[commit-bot: I haz the power, 2017-01-07 00:58:25]

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

[commit-bot: I haz the power, 2017-01-07 00:58:26]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-01-09 16:01:33]

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:374: // If the
statement is not an expression then it's a constant.
On 2017/01/07 00:50:18, dcheng wrote:
> On 2017/01/07 00:48:00, danakj_OOO_and_slow wrote:
> > On 2017/01/07 00:45:49, dcheng wrote:
> > > What would it be that makes it a constant?
> > 
> > switch, if, return, language stuff. Not things that change at runtime.
> 
> I think we always pass in an Expr though, no? So this should just take an
Expr,
> and we can remove this if (it doesn't make sense for any of those to be
> initializers, AFAIK)

Children of the expr are stmts though.

[danakj, 2017-01-09 16:01:51]

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2616213003/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:390: if
(!CanBeEvaluatedAtCompileTime(child, context))
IE here child is not an expr always.

[danakj, 2017-01-09 16:01:56]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2017-01-09 16:02:26]

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

[danakj, 2017-01-09 16:23:45]

Description was changed from

==========
Get more consistent decisions if a statement is constant in templates

The tool was saying some template-dependent values were both const
and not const because when the template parameters meet some criteria
it collapses them out of the AST. If we always check the entire
subtree of expressions to verify they are evaluatable we get more
consistent results.

Also don't try to turn this_type_of_name into kThisTypeOfName, as
the other explicitly wrote it that way and we end up with the ever
fun kThis_type_of_name instead.

R=dcheng@chromium.org
BUG=677285
==========

to

==========
Get more consistent decisions if a statement is constant in templates

The tool was saying some template-dependent values were both const
and not const because when the template parameters meet some criteria
it collapses them out of the AST. If we always check the entire
subtree of expressions to verify they are evaluatable we get more
consistent results.

Also don't try to turn this_type_of_name into kThisTypeOfName, as
the author explicitly wrote it that way and we end up with the ever
fun kThis_type_of_name instead.

R=dcheng@chromium.org
BUG=677285
==========

[danakj, 2017-01-09 16:23:51]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2017-01-09 16:23:55]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2017-01-09 16:24:22]

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-01-09 16:29:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483979035460640,
"parent_rev": "06318a315afd262552ee1393cf360f4adb27223a", "commit_rev":
"b9a1485ee7f50c3980f8055d8f64ea4d30220720"}

[commit-bot: I haz the power, 2017-01-09 16:30:06]

Description was changed from

==========
Get more consistent decisions if a statement is constant in templates

The tool was saying some template-dependent values were both const
and not const because when the template parameters meet some criteria
it collapses them out of the AST. If we always check the entire
subtree of expressions to verify they are evaluatable we get more
consistent results.

Also don't try to turn this_type_of_name into kThisTypeOfName, as
the author explicitly wrote it that way and we end up with the ever
fun kThis_type_of_name instead.

R=dcheng@chromium.org
BUG=677285
==========

to

==========
Get more consistent decisions if a statement is constant in templates

The tool was saying some template-dependent values were both const
and not const because when the template parameters meet some criteria
it collapses them out of the AST. If we always check the entire
subtree of expressions to verify they are evaluatable we get more
consistent results.

Also don't try to turn this_type_of_name into kThisTypeOfName, as
the author explicitly wrote it that way and we end up with the ever
fun kThis_type_of_name instead.

R=dcheng@chromium.org
BUG=677285

Review-Url: https://codereview.chromium.org/2616213003
Cr-Commit-Position: refs/heads/master@{#442262}
Committed:
https://chromium.googlesource.com/chromium/src/+/b9a1485ee7f50c3980f8055d8f64...
==========

[commit-bot: I haz the power, 2017-01-09 16:30:07]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b9a1485ee7f50c3980f8055d8f64...