[PATCH] D44589: [Sema] Make deprecation fix-it replace all multi-parameter ObjC method slots.
Erik Pilkington via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 21 10:41:44 PDT 2018
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
More information about the cfe-commits
mailing list