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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked 2 inline comments as done.
vsapsai added inline comments.


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


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6934-6938
+// Returns a number of method parameters if parsing is successful.
+// In case of unsuccessful parsing SlotNames can contain invalid data.
+static Optional<unsigned>
+parseObjCMethodName(StringRef Name, SmallVectorImpl<StringRef> &SlotNames,
+                    const LangOptions &LangOpts) {
----------------
erik.pilkington wrote:
> 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.
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.


https://reviews.llvm.org/D44589