[PATCH] D116417: [clang][ObjC] Add fix it for missing methods in impl

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks good!



================
Comment at: clang/lib/Sema/SemaDeclObjC.cpp:2237
+    // a better place to put it.
+    std::string Str;
+    llvm::raw_string_ostream Out(Str);
----------------
Str -> FixitStr

The comment is IMO too verbose, maybe just "// Add an empty definition at the end of @implementation".
(The future idea doesn't seem either specific or critical)


================
Comment at: clang/lib/Sema/SemaDeclObjC.cpp:2239
+    llvm::raw_string_ostream Out(Str);
+    PrintingPolicy Policy(Impl->getASTContext().getPrintingPolicy());
+    method->print(Out, Policy);
----------------
No need to copy the policy, just refer to it directly?


================
Comment at: clang/lib/Sema/SemaDeclObjC.cpp:2241
+    method->print(Out, Policy);
+    Str.append(" {\n\n}\n\n");
+
----------------
nit: mixing string & output stream modifications is a bit confusing


================
Comment at: clang/lib/Sema/SemaDeclObjC.cpp:2241
+    method->print(Out, Policy);
+    Str.append(" {\n\n}\n\n");
+
----------------
sammccall wrote:
> nit: mixing string & output stream modifications is a bit confusing
I think this should probably only have one inside and one at the end.

Fixits aren't really in the business of formatting the code to be further modified.
I can't find examples of fixits inserting blank lines that aren't necessary, apart from at the end of decls.

One newline between empty braces seems to be LLVM's style for objc. (For C++ it's zero newlines)


================
Comment at: clang/test/FixIt/fixit-objc-missing-method-impl.m:1
+// RUN: cp %s %t
+// RUN: not %clang_cc1 -pedantic -Werror -fixit -x objective-c %t
----------------
Can you also include a -verify run to show the error being fixed? This seems to be common in other tests, and it makes it much easier to understand what's being tested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116417/new/

https://reviews.llvm.org/D116417