[PATCH] D69543: [clangd] Add a tweak refactoring to wrap Objective-C string literals in `NSLocalizedString` macros

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

implementation lgtm with a few nits.

main concern is about the new getlangopts helper



================
Comment at: clang-tools-extra/clangd/ParsedAST.h:80
 
+  const LangOptions &getLangOpts() const {
+    return getASTContext().getLangOpts();
----------------
can we introduce this helper in a new patch, while changing other occurrences in clangd?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp:34
+/// After:
+///   NSLocalizedString(@"description", "")
+class ObjCLocalizeStringLiteral : public Tweak {
----------------
 NSLocalizedString(@"description", `@`"")


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp:69
+      SM, CharSourceRange::getCharRange(Str->getBeginLoc()),
+      "NSLocalizedString(", Inputs.AST.getASTContext().getLangOpts()));
+  SourceLocation EndLoc = Lexer::getLocForEndOfToken(
----------------
maybe extract `Inputs.AST.getASTContext().getLangOpts()` into a variable and make use of it in the following places as well? (line 72 and 75)


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:131
+  // Ensure the the action can be initiated in the string literal.
+  EXPECT_AVAILABLE(R"(id x = ^@^"^t^est^";)");
+  EXPECT_AVAILABLE(R"(id x = [[@"test"]];)");
----------------
nit: you can combine all of this into a single test


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:136
+  // Ensure that the action can't be initiated in other places.
+  EXPECT_UNAVAILABLE(R"(i^d ^x ^= @"test";^)");
+  EXPECT_UNAVAILABLE(R"(id [[x]] = @"test";)");
----------------
nit: you can combine all of this into a single test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69543