[PATCH] D56924: Special case ObjCPropertyDecl for printing

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.



Comment at: unittests/AST/NamedDeclPrinterTest.cpp:220
+"property",
+"Obj::property"));
+}

dexonsmith wrote:
> gribozavr wrote:
> > I don't think that `Obj::property` is the preferred syntax. `Obj.property`? 
> > I'd want a review from someone from Apple to confirm.
> @arphaman, @jkorous, or @benlangmuir: can you check on this?
For more context here: I'm seeing similar issues for Objective-C instance 
variables (they can be declared inside a class extension --> meaning the Decl 
has no name --> clangd fails with an assertion that the name shouldn't be 
empty).

It seems to me that for Objective-C we could do something like 
`Obj(Extension::property` or alternatively `Obj(Extension).property` and 
`Obj(class extension)->_instanceVar`, but I'm not sure about what relies upon 
this current behavior (if anything).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added subscribers: jkorous, benlangmuir.
dexonsmith added inline comments.



Comment at: unittests/AST/NamedDeclPrinterTest.cpp:220
+"property",
+"Obj::property"));
+}

gribozavr wrote:
> I don't think that `Obj::property` is the preferred syntax. `Obj.property`? 
> I'd want a review from someone from Apple to confirm.
@arphaman, @jkorous, or @benlangmuir: can you check on this?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: lib/AST/Decl.cpp:1543
+Ctx = ID;
+  }
 

Like you said in a private conversation, yes, support for `ObjCIvarDecl` also 
seems necessary.



Comment at: unittests/AST/NamedDeclPrinterTest.cpp:220
+"property",
+"Obj::property"));
+}

I don't think that `Obj::property` is the preferred syntax. `Obj.property`? I'd 
want a review from someone from Apple to confirm.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[David Goldman via Phabricator via cfe-commits]

dgoldman closed this revision.
dgoldman added a comment.

Closed via rL357720 


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 193769.
dgoldman added a comment.

Rebase


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924

Files:
  lib/AST/Decl.cpp
  unittests/AST/NamedDeclPrinterTest.cpp


Index: unittests/AST/NamedDeclPrinterTest.cpp
===
--- unittests/AST/NamedDeclPrinterTest.cpp
+++ unittests/AST/NamedDeclPrinterTest.cpp
@@ -115,6 +115,18 @@
  "input.cc");
 }
 
+::testing::AssertionResult
+PrintedWrittenPropertyDeclObjCMatches(StringRef Code, StringRef DeclName,
+   StringRef ExpectedPrinted) {
+  std::vector Args{"-std=c++11", "-xobjective-c++"};
+  return PrintedNamedDeclMatches(Code,
+ Args,
+ /*SuppressUnwrittenScope*/ true,
+ 
objcPropertyDecl(hasName(DeclName)).bind("id"),
+ ExpectedPrinted,
+ "input.m");
+}
+
 } // unnamed namespace
 
 TEST(NamedDeclPrinter, TestNamespace1) {
@@ -179,3 +191,31 @@
 "A",
 "X::A"));
 }
+
+TEST(NamedDeclPrinter, TestObjCClassExtension) {
+  ASSERT_TRUE(PrintedWrittenPropertyDeclObjCMatches(
+R"(
+  @interface Obj
+  @end
+
+  @interface Obj ()
+  @property(nonatomic) int property;
+  @end
+)",
+"property",
+"Obj::property"));
+}
+
+TEST(NamedDeclPrinter, TestObjCClassExtensionWithGetter) {
+  ASSERT_TRUE(PrintedWrittenPropertyDeclObjCMatches(
+R"(
+  @interface Obj
+  @end
+
+  @interface Obj ()
+  @property(nonatomic, getter=myPropertyGetter) int property;
+  @end
+)",
+"property",
+"Obj::property"));
+}
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -1531,10 +1531,16 @@
const PrintingPolicy ) const {
   const DeclContext *Ctx = getDeclContext();
 
-  // For ObjC methods, look through categories and use the interface as 
context.
+  // For ObjC methods and properties, look through categories and use the
+  // interface as context.
   if (auto *MD = dyn_cast(this))
 if (auto *ID = MD->getClassInterface())
   Ctx = ID;
+  if (auto *PD = dyn_cast(this)) {
+if (auto *MD = PD->getGetterMethodDecl())
+  if (auto *ID = MD->getClassInterface())
+Ctx = ID;
+  }
 
   if (Ctx->isFunctionOrMethod()) {
 printName(OS);


Index: unittests/AST/NamedDeclPrinterTest.cpp
===
--- unittests/AST/NamedDeclPrinterTest.cpp
+++ unittests/AST/NamedDeclPrinterTest.cpp
@@ -115,6 +115,18 @@
  "input.cc");
 }
 
+::testing::AssertionResult
+PrintedWrittenPropertyDeclObjCMatches(StringRef Code, StringRef DeclName,
+   StringRef ExpectedPrinted) {
+  std::vector Args{"-std=c++11", "-xobjective-c++"};
+  return PrintedNamedDeclMatches(Code,
+ Args,
+ /*SuppressUnwrittenScope*/ true,
+ objcPropertyDecl(hasName(DeclName)).bind("id"),
+ ExpectedPrinted,
+ "input.m");
+}
+
 } // unnamed namespace
 
 TEST(NamedDeclPrinter, TestNamespace1) {
@@ -179,3 +191,31 @@
 "A",
 "X::A"));
 }
+
+TEST(NamedDeclPrinter, TestObjCClassExtension) {
+  ASSERT_TRUE(PrintedWrittenPropertyDeclObjCMatches(
+R"(
+  @interface Obj
+  @end
+
+  @interface Obj ()
+  @property(nonatomic) int property;
+  @end
+)",
+"property",
+"Obj::property"));
+}
+
+TEST(NamedDeclPrinter, TestObjCClassExtensionWithGetter) {
+  ASSERT_TRUE(PrintedWrittenPropertyDeclObjCMatches(
+R"(
+  @interface Obj
+  @end
+
+  @interface Obj ()
+  @property(nonatomic, getter=myPropertyGetter) int property;
+  @end
+)",
+"property",
+"Obj::property"));
+}
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -1531,10 +1531,16 @@
const PrintingPolicy ) const {
   const DeclContext *Ctx = getDeclContext();
 
-  // For ObjC methods, look through categories and use the interface as context.
+  // For ObjC methods and properties, look through categories and use the
+  // interface as context.
   if (auto *MD = dyn_cast(this))
 if (auto *ID = MD->getClassInterface())
   Ctx = ID;
+  if (auto *PD = dyn_cast(this)) {
+if (auto *MD = PD->getGetterMethodDecl())
+  if (auto *ID = MD->getClassInterface())
+Ctx = ID;
+  }
 
   if (Ctx->isFunctionOrMethod()) {
 printName(OS);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Sorry, I missed the pings. It LGTM.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.
Herald added a subscriber: dexonsmith.

ping


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

ping


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

ping


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

friendly ping, think this is good to go now


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56924: Special case ObjCPropertyDecl for printing

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

In D56924#1407836 , @arphaman wrote:

> Please add a test that covers the '(class extension)' output as well.


Removed the `(class extension)` output as the property getter if statement 
should now handle this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56924/new/

https://reviews.llvm.org/D56924



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits