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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

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



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


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


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6959-6966
+    if (S.empty())
+      continue;
+    if (!isIdentifierHead(S[0], AllowDollar))
+      return None;
+    for (char C : S)
+      if (!isIdentifierBody(C, AllowDollar))
+        return None;
----------------
Might be better to add a defaulted AllowDollar parameter to isValidIdentifier() in CharInfo.h and use that rather than duplicating it here.


https://reviews.llvm.org/D44589