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