New issue
Advanced search Search tips

Issue 640338 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 640016
Owner:
Closed: Aug 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

rewrite_to_chrome_style: wtf::MutexBase::lock is not rewritten to Lock

Project Member Reported by lukasza@chromium.org, Aug 23 2016

Issue description

third_party/WebKit/Source/wtf/ThreadingPrimitives.h got rewritten as follows:

    class WTF_EXPORT MutexBase {
        WTF_MAKE_NONCOPYABLE(MutexBase); USING_FAST_MALLOC(MutexBase);
    public:
        ~MutexBase();
    
        void lock();
        void unlock();
    #if ENABLE(ASSERT)
        bool Locked() { return mutex_.recursion_count_ > 0; }
    #endif

Note that |locked| got rewritten to |Locked|, but |lock| and |unlocked| where not rewritten here.  This is a problem, because |lock| *did* get rewritten in third_party/WebKit/Source/wtf/Locker.h:

    template <typename T> class Locker final {
        STACK_ALLOCATED();
        WTF_MAKE_NONCOPYABLE(Locker);
    public:
        Locker(T& lockable) : lockable_(lockable) { lockable_.Lock(); }
        ~Locker() { lockable_.Unlock(); }
    private:
        T& lockable_;
    };

Note that the repro above happens only after the following 3 CLs are applied:
- https://codereview.chromium.org/2246263002
- https://codereview.chromium.org/2274653002
- https://codereview.chromium.org/2256913002
Without the CLs above, |lock| inside Locker.h doesn't get rewritten (and so inconsistent rewriting in ThreadingPrimitives.h is not a compile problem).  I am guessing that |lock| inside Locker.h is a CXXDependentScopeMemberExpr (which starts getting rewritten in https://codereview.chromium.org/2256913002).
 
Hmmm... it seems that "lock" and "unlock" are blacklisted in IsBlacklistedMethod in RewriteToChromeStyle.cpp.  Okay - I can take that into account in  https://codereview.chromium.org/2274653002 (which introduces GuessNameForUnresolvedDependentNode responsible for renaming within |Locker|).

Comment 2 by danakj@chromium.org, Aug 23 2016

Ah ya thats for interacting with STL stuff. If this never does you could whitelist them..  if you want :P

Comment 3 by 520112...@gmail.com, Aug 23 2016

所以這個是!?

Comment 4 by danakj@chromium.org, Aug 23 2016

Google translate suggests you're asking what this is, you can see the blocking bug for a higher level idea https://bugs.chromium.org/p/chromium/issues/detail?id=578344
Mergedinto: 640016
Status: Duplicate (was: Untriaged)
This is taken care of by https://codereview.chromium.org/2274653002/#ps100001, which starts to obey blacklists for unresolved / decl-less nodes.  Therefore I think I'll lazily resolve this bug as a dupe of 640016 (and for now won't follow the whitelisting suggestion from #c2 - AFAICT Locker is not used with std namespace today, but the way it is written allows [AFAIK] substituting T=std::mutex).

Comment 6 by danakj@chromium.org, Aug 24 2016

Cool SGTM

Sign in to add a comment