This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Make deprecation fix-it replace all multi-parameter ObjC method slots.
ClosedPublic

Authored by vsapsai on Mar 16 2018, 3:08 PM.

Details

Summary

Deprecation replacement can be any text but if it looks like a name of
ObjC method and has the same number of arguments as original method,
replace all slot names so after applying a fix-it you have valid code.

rdar://problem/36660853

Diff Detail

Repository
rL LLVM

Event Timeline

vsapsai created this revision.Mar 16 2018, 3:08 PM
aaron.ballman added inline comments.
clang/include/clang/Sema/DelayedDiagnostic.h
198–199 ↗(On Diff #138779)

You can use makeArrayRef() here instead of the explicit template argument constructor.

clang/lib/Sema/SemaDeclAttr.cpp
7196 ↗(On Diff #138779)

Do not use auto here, as the type is not explicitly spelled out in the initialization.

7207–7217 ↗(On Diff #138779)

You can elide a lot of braces here.

vsapsai added inline comments.Mar 16 2018, 3:55 PM
clang/lib/Sema/SemaDeclAttr.cpp
7207–7217 ↗(On Diff #138779)

Just to clarify, is it OK if if-branch is with braces but else-branch without? I.e.

if (...) {
  line 1
  line 2
} else
  the only line
aaron.ballman added inline comments.Mar 16 2018, 4:27 PM
clang/lib/Sema/SemaDeclAttr.cpp
7207–7217 ↗(On Diff #138779)

We're consistently inconsistent here. :-) It's okay to do so, and it's okay to not do it, I only brought it up because of how many braces we could elide. It's your call, though.

vsapsai updated this revision to Diff 139233.Mar 20 2018, 5:43 PM

Address review comments:

  • Replace auto with explicit type.
  • Use llvm::makeArrayRef.
  • Remove curly braces around single-statement elses.
vsapsai marked 3 inline comments as done.Mar 20 2018, 5:48 PM
vsapsai added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
7207–7217 ↗(On Diff #138779)

I know I like braces too much, more than code style mandates, so I removed them. Hopefully, that reflects code style better. And I tried to clang-format both options - no changes.

This generally looks reasonable to me, but @rsmith should weigh in before you commit because MultiSourceLocation is novel.

This generally looks reasonable to me, but @rsmith should weigh in before you commit because MultiSourceLocation is novel.

Thanks for the review, Aaron. I tried not to do anything stupid with MultiSourceLocation but another opinion will be definitely useful.

Hi Volodymyr, thanks for working on this! Overall this looks good, I just have a few nits.

clang/include/clang/Basic/SourceLocation.h
202 ↗(On Diff #139233)

Why can't we just use an ArrayRef<SourceLocation> for this? It looks like that type already has a converting constructor from SourceLocation, so we should be able to use in in DiagnoseUseOfDecl without any noise.

clang/lib/Sema/SemaDeclAttr.cpp
6934–6938 ↗(On Diff #139233)

Maybe tryParseReplacementObjCMethodName or something would be better? parseObjCMethodName() is pretty vague. Also the comment above should probably be a /// doxygen comment. It would be nice if you mentioned that Name originated from an availability attribute in the comment.

6959–6966 ↗(On Diff #139233)

Might be better to add a defaulted AllowDollar parameter to isValidIdentifier() in CharInfo.h and use that rather than duplicating it here.

vsapsai updated this revision to Diff 139650.Mar 23 2018, 2:01 PM

Address review comments:

  • update comments for tryParseObjCMethodName, use isValidIdentifier.
  • make MultiSourceLocation more lightweight.

I have rebased my patch, so diff between changes can be noisy.

vsapsai marked 2 inline comments as done.Mar 23 2018, 2:18 PM
vsapsai added inline comments.
clang/include/clang/Basic/SourceLocation.h
202 ↗(On Diff #139233)

I've tried to go middle way this time and have MultiSourceLocation just as typedef. I modeled it after MultiExprArg which is using MultiExprArg = MutableArrayRef<Expr *>; But that approach can have different reasons and isn't necessarily applicable in this case.

My problem here is that on one hand I think MultiSourceLocation might be a useful abstraction and in that case probably Sema::BuildInstanceMessage should use this type for its parameter. On the other hand I am struggling to come up with good explanation what MultiSourceLocation is. And that's an indication it's not a good abstraction. What do you think?

clang/lib/Sema/SemaDeclAttr.cpp
6934–6938 ↗(On Diff #139233)

I like "try" part. But not sure about "Replacement". This function doesn't care how its output will be used so I don't think it is worth mentioning.

I've updated comment for Name but not entirely satisfied with the wording. Will try to come up with something better and suggestions are welcome.

erik.pilkington accepted this revision.Mar 26 2018, 6:08 PM

LGTM, this is a really nice feature!

clang/include/clang/Basic/SourceLocation.h
202 ↗(On Diff #139233)

It does looks like this abstraction is used a lot in the sema for objective-c. I would probably rather leave it named ArrayRef<SourceLocation> because that is just as informative a name as MultiSourceLocation, but also makes it clear what the actual type is (makes it more obvious who owns the underlying array). I think this is fine either way though, if you'd rather add the typedef.

clang/lib/Sema/DelayedDiagnostic.cpp
49 ↗(On Diff #139650)

You should probably move this assert up before you call .front()!

This revision is now accepted and ready to land.Mar 26 2018, 6:08 PM
vsapsai updated this revision to Diff 139981.Mar 27 2018, 12:33 PM
  • More tweaks. Remove MultiSourceLocation as nobobody, including me, sees much value in it.
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.