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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
