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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington accepted this revision.
erik.pilkington added a comment.
This revision is now accepted and ready to land.

LGTM, this is a really nice feature!



================
Comment at: clang/include/clang/Basic/SourceLocation.h:202
+/// Can be used transparently in places where SourceLocation is expected.
+class MultiSourceLocation {
+  bool IsSingleLoc;
----------------
vsapsai wrote:
> erik.pilkington wrote:
> > 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.
> 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?
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.


================
Comment at: clang/lib/Sema/DelayedDiagnostic.cpp:49
+
+  assert(!Locs.empty());
+  DD.AvailabilityData.SelectorLocs = new SourceLocation[Locs.size()];
----------------
You should probably move this assert up before you call `.front()`!


https://reviews.llvm.org/D44589