New issue
Advanced search Search tips

Issue 670434 link

Starred by 1 user
Status: Fixed
Owner:
lukasza@chromium.org
Closed: Dec 2016
Cc:
danakj@chromium.org
dcheng@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

rewrite_to_chrome_style: Call to WTF::HashMap<K, V>::clear doesn't get rewritten

Project Member Reported by lukasza@chromium.org, Dec 1 2016 
|clear| doesn't get rewritten to |Clear| here:

mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:

    ...
    #include "third_party/WebKit/Source/wtf/HashMap.h"

    namespace mojo {

    template <typename K, typename V>
    struct MapTraits<WTF::HashMap<K, V>> {
    ...
      static void SetToEmpty(WTF::HashMap<K, V>* output) { output->clear(); }  <- HERE
    };

    }  // namespace mojo

FWIW, I can locally repro with an extra test in tools/clang/rewrite_to_chrome_style/tests.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 2 2016 

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

commit 2f5f4c91d9b4d3087533b7e2068d5e81c989c933
Author: lukasza <lukasza@chromium.org>
Date: Fri Dec 02 20:05:28 2016

Handling ElaboratedType and InjectedClassNameType in blink_qual_type_matcher.

ElaboratedType wasn't handled, because of a bug in hasDeclaration
matcher.  We had a workaround in definition of
blink_qual_type_base_matcher, but this didn't fully work (because I
forgot to use this in pointsTo and references).

At any rate, the right way forward is to use hasUnqualifiedDesugaredType
matcher, which has landed in https://reviews.llvm.org/D27207 (aka
rL288366) in upstream clang.  This is the right way, because
1) hasUnqualifiedDesugaredType will desugar not only through
   ElaboratedType, but also through TypedefAliasType (which is a good
   thing, because we don't want to rename in case of
   variableOfTypeAliasDefinedInBlinkButPointingToNonBlinkType->dontRename()).
2) Another pending clang change (https://reviews.llvm.org/D27104) will
   remove desugaring from hasDeclaration(type/qualType) - this CL
   insulates us from this change.

The changes described above fix  https://crbug.com/670434 .

It turns out that after the fix we also need to explicitly handle
hopping over InjectedClassNameType, which luckily was caught by one of
already existing tests in function-templates-original.cc (the type of
|this| in constructor of Checked is an InjectedClassNameType).

BUG= 670434 

Review-Url: https://codereview.chromium.org/2544933002
Cr-Commit-Position: refs/heads/master@{#436004}

[modify] https://crrev.com/2f5f4c91d9b4d3087533b7e2068d5e81c989c933/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
[modify] https://crrev.com/2f5f4c91d9b4d3087533b7e2068d5e81c989c933/tools/clang/rewrite_to_chrome_style/tests/template-expected.cc
[modify] https://crrev.com/2f5f4c91d9b4d3087533b7e2068d5e81c989c933/tools/clang/rewrite_to_chrome_style/tests/template-original.cc

Comment 2 by lukasza@chromium.org, Dec 2 2016

Status: Fixed (was: Started)
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 8 2016 

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

commit 864789b24b99adb37737475ae5315326bc78fa80
Author: lukasza <lukasza@chromium.org>
Date: Thu Dec 08 20:34:32 2016

hasUnqualifiedDesugaredType now comes with clang - stop defining it outselves.

BUG= 670434 

Review-Url: https://codereview.chromium.org/2563733002
Cr-Commit-Position: refs/heads/master@{#437321}

[modify] https://crrev.com/864789b24b99adb37737475ae5315326bc78fa80/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
►

Sign in to add a comment