New issue
Advanced search Search tips

Issue 669937 link

Starred by 1 user
Status: Duplicate
Merged: issue 643779
Owner:
lukasza@chromium.org
Closed: Nov 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: ElaboratedType vs CXXDependentScopeMemberExpr trouble

Project Member Reported by lukasza@chromium.org, Nov 30 2016 
https://codereview.chromium.org/2256913002 takes care of rewriting CXXDependentScopeMemberExpr, but in some cases has trouble deciding if the unresolved type comes from the Blink namespace, because hasDeclaration doesn't hop over ElaboratedType (Type vs ::blink::Type) - https://llvm.org/bugs/show_bug.cgi?id=30331.  See also:
- https://reviews.llvm.org/D24361 (abandoned)
- https://reviews.llvm.org/D27104 (the right way forward)
- https://reviews.llvm.org/D27207 (idea for how to fix rewrite_to_chrome_style in the meantime)

Example of code that doesn't get properly rewritten:

third_party/WebKit/Source/platform/mojo/SecurityOriginStructTraits.h:
(line numbering below doesn't correspond in any way to the real line numbers in the source file)

1  namespace mojo {
2  
3  template <>
4  struct StructTraits<url::mojom::blink::Origin::DataView,
5                      RefPtr<::blink::SecurityOrigin>> {
6    static WTF::String scheme(const RefPtr<::blink::SecurityOrigin>& origin) {
7      return origin->protocol();
8    }

Notes:
- Notice usage of ElaboratedType on lines 5 and 6 (i.e. notice the |::blink::| prefix)
- The bug is that |protocol| on line 7 is not getting rewritten to |Protocol|.

Comment 1 by lukasza@chromium.org, Nov 30 2016 

A narrowed down target to repro is:

ninja -C out/gn -j 10 -k 9999 obj/content/common/mojo_bindings_blink_cpp_sources/frame.mojom-blink.o

(this also a similar problem in other FooStructTraits.h like KURLStructTraits.h).

Comment 2 by lukasza@chromium.org, Nov 30 2016 

I have no idea why this issue doesn't repro when I try to reproduce it via something that (I think) looks almost identical:

--- a/tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc
+++ b/tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc
@@ -169,7 +169,7 @@ namespace blink {
 
 struct StructInBlink {
   // Structs in blink should rename their methods to capitals.
-  bool Function() { return true; }
+  bool Function() const { return true; }
 };
 
 class BitVector {
@@ -206,8 +206,35 @@ struct StructInWTF {
   bool Function() { return true; }
 };
 
+template <typename T>
+class MyRefPtr2 {
+ public:
+  T* operator->() const { return nullptr; }
+};
+
 }  // namespace WTF
 
+// Mimicking the using declaration from wtf/RefPtr.h
+using WTF::MyRefPtr2;
+
+namespace elaborated_type_repro {
+
+template <typename T1, typename T2>
+struct StructTraits;
+
+template <>
+struct StructTraits<int, MyRefPtr2<::blink::StructInBlink>> {
+  static bool scheme(const MyRefPtr2<::blink::StructInBlink>& origin) {
+    // Need to rename |function| to |Function| below - the usage of
+    // clang::ElaboratedType above (|::blink::StructInBlink| vs
+    // |StructInBlink|) had caused a bug here.  See also:
+    //  https://crbug.com/669937 .
+    return origin->Function();
+  }
+};
+
+}  // namespace elaborated_type_repro
+

Comment 3 by lukasza@chromium.org, Nov 30 2016

Mergedinto: 643779
Status: Duplicate (was: Started)
►

Sign in to add a comment