Page MenuHomePhabricator

Special case ObjCPropertyDecl for printing
ClosedPublic

Authored by dgoldman on Jan 18 2019, 11:29 AM.

Details

Summary

ObjCPropertyDecl should use the category interface as a context similar to what is done for methods.

Previously category methods would be printed as ::property; now they are printed as Class::property.

Diff Detail

Event Timeline

dgoldman created this revision.Jan 18 2019, 11:29 AM
dgoldman updated this revision to Diff 182971.Jan 22 2019, 1:05 PM
  • Add test

This is definitely an improvement, though I don't know if it's *right*. @akyrtzi, thoughts?

unittests/AST/NamedDeclPrinterTest.cpp
206

One concern is that similarly-named members in extensions of different classes will have the same name (whereas categories have names, so the collision is less likely).
It's also harder to go from printed name -> source code.

Obj::(class extension)::property might be a more useful QName (including *also* the class name).
But I don't know obj-c well, would be good if Apple folks could chime in.

(class extension)::property is definitely an improvement over the current ::property, though.

This is definitely an improvement, though I don't know if it's *right*. @akyrtzi, thoughts?

Yeah, I'm not sure what the desired behavior is. When writing up the test I noticed there is a Obj::property, presumably for the auto-generated getter method (which I then filtered out via objcPropertyDecl).

Where is the qualified name used?

arphaman requested changes to this revision.Jan 31 2019, 1:06 PM
arphaman added a subscriber: arphaman.

Do you know if this have an effect on the output of completion results or other tooling-based output?

A couple of requests:

  • This behavior should be controlled by a printing policy flag SupressUnwrittenScope (specifically for the '::(class extension)').
  • I also agree with Sam's comment. The property should still be qualified by the class name as well, e.g. Obj::(class extension)::property.
This revision now requires changes to proceed.Jan 31 2019, 1:06 PM
dgoldman updated this revision to Diff 185341.Feb 5 2019, 10:07 AM
  • Output class scope for ObjCPropertyDecl
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 10:07 AM

Do you know if this have an effect on the output of completion results or other tooling-based output?

A couple of requests:

  • This behavior should be controlled by a printing policy flag SupressUnwrittenScope (specifically for the '::(class extension)').
  • I also agree with Sam's comment. The property should still be qualified by the class name as well, e.g. Obj::(class extension)::property.

It's possible - clangd for instance was crashing because of the current behavior: ::property instead of Obj::property or (class extension)::property.

  • Added although see below
  • I've modified this to be more in line with the current handling of Objc methods, but this no longer outputs the "(class extension)", do you think it should output both?

ping, looking to get this in to fix a clangd assertion failure

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

dgoldman updated this revision to Diff 188213.Feb 25 2019, 10:21 AM
  • Remove (class extension) as it's no longer needed
dgoldman retitled this revision from Handle ObjCCategoryDecl class extensions for print to Special case ObjCPropertyDecl for printing.EditedFeb 25 2019, 10:24 AM
dgoldman edited the summary of this revision. (Show Details)

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.

friendly ping, think this is good to go now

Sorry, I missed the pings. It LGTM.

arphaman accepted this revision.Apr 2 2019, 6:15 PM
This revision is now accepted and ready to land.Apr 2 2019, 6:15 PM
gribozavr added inline comments.
lib/AST/Decl.cpp
1543

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

unittests/AST/NamedDeclPrinterTest.cpp
220

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

dexonsmith added inline comments.
unittests/AST/NamedDeclPrinterTest.cpp
220

@arphaman, @jkorous, or @benlangmuir: can you check on this?

dgoldman added inline comments.Fri, Oct 18, 9:00 AM
unittests/AST/NamedDeclPrinterTest.cpp
220

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).