[PATCH] D69543: [clangd] Add a tweak refactoring to wrap Objective-C string literals in `NSLocalizedString` macros
This revision was automatically updated to reflect the committed changes.
arphaman marked an inline comment as done.
Closed by commit rG27f124445755: [clangd] Add a tweak refactoring to wrap
Objective-C string literals in… (authored by arphaman).
Changed prior to commit:
https://reviews.llvm.org/D69543?vs=231799&id=232238#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69543/new/
https://reviews.llvm.org/D69543
Files:
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -122,6 +122,25 @@
EXPECT_EQ(apply(Input), Output);
}
+TWEAK_TEST(ObjCLocalizeStringLiteral);
+TEST_F(ObjCLocalizeStringLiteralTest, Test) {
+ ExtraArgs.push_back("-x");
+ ExtraArgs.push_back("objective-c");
+
+ // Ensure the the action can be initiated in the string literal.
+ EXPECT_AVAILABLE(R"(id x = ^[[@[[^"^t^est^";)");
+
+ // Ensure that the action can't be initiated in other places.
+ EXPECT_UNAVAILABLE(R"([[i^d ^[[x]] ^= @"test";^]])");
+
+ // Ensure that the action is not available for regular C strings.
+ EXPECT_UNAVAILABLE(R"(const char * x= "^test";)");
+
+ const char *Input = R"(id x = [[@"test"]];)";
+ const char *Output = R"(id x = NSLocalizedString(@"test", @"");)";
+ EXPECT_EQ(apply(Input), Output);
+}
+
TWEAK_TEST(DumpAST);
TEST_F(DumpASTTest, Test) {
EXPECT_AVAILABLE("^int f^oo() { re^turn 2 ^+ 2; }");
Index: clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
@@ -0,0 +1,85 @@
+//===--- ObjcLocalizeStringLiteral.cpp ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Logger.h"
+#include "ParsedAST.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ExprObjC.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+/// Wraps an Objective-C string literal with the NSLocalizedString macro.
+/// Before:
+/// @"description"
+/// ^^^
+/// After:
+/// NSLocalizedString(@"description", @"")
+class ObjCLocalizeStringLiteral : public Tweak {
+public:
+ const char *id() const override final;
+ Intent intent() const override { return Intent::Refactor; }
+
+ bool prepare(const Selection &Inputs) override;
+ Expected apply(const Selection &Inputs) override;
+ std::string title() const override;
+
+private:
+ const clang::ObjCStringLiteral *Str = nullptr;
+};
+
+REGISTER_TWEAK(ObjCLocalizeStringLiteral)
+
+bool ObjCLocalizeStringLiteral::prepare(const Selection &Inputs) {
+ const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
+ if (!N)
+return false;
+ // Allow the refactoring even if the user selected only the C string part
+ // of the expression.
+ if (N->ASTNode.get()) {
+if (N->Parent)
+ N = N->Parent;
+ }
+ Str = dyn_cast_or_null(N->ASTNode.get());
+ return Str;
+}
+
+Expected
+ObjCLocalizeStringLiteral::apply(const Selection &Inputs) {
+ auto &SM = Inputs.AST.getSourceManager();
+ auto &LangOpts = Inputs.AST.getASTContext().getLangOpts();
+ auto Reps = tooling::Replacements(tooling::Replacement(
+ SM, CharSourceRange::getCharRange(Str->getBeginLoc()),
+ "NSLocalizedString(", LangOpts));
+ SourceLocation EndLoc = Lexer::getLocForEndOfToken(
+ Str->getEndLoc(), 0, Inputs.AST.getSourceManager(), LangOpts);
+ if (auto Err = Reps.add(tooling::Replacement(
+ SM, CharSourceRange::getCharRange(EndLoc), ", @\"\")", LangOpts)))
+return std::move(Err);
+ return Effect::mainFileEdit(SM, std::move(Reps));
+}
+
+std::string ObjCLocalizeStringLiteral::title() const {
+ return "Wrap in NSLocalizedString";
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===
--- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-ex
[PATCH] D69543: [clangd] Add a tweak refactoring to wrap Objective-C string literals in `NSLocalizedString` macros
arphaman marked 2 inline comments as done.
arphaman added inline comments.
Comment at: clang-tools-extra/clangd/ParsedAST.h:80
+ const LangOptions &getLangOpts() const {
+return getASTContext().getLangOpts();
kadircet wrote:
> arphaman wrote:
> > kadircet wrote:
> > > can we introduce this helper in a new patch, while changing other
> > > occurrences in clangd?
> > Sounds good, I'll do that.
> can you also revert this change?
I precommited it in c0ee0224c4cf52bc6ba74dec88b30b850deca523
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69543/new/
https://reviews.llvm.org/D69543
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69543: [clangd] Add a tweak refactoring to wrap Objective-C string literals in `NSLocalizedString` macros
kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
Thanks, LGTM!
Comment at: clang-tools-extra/clangd/ParsedAST.h:80
+ const LangOptions &getLangOpts() const {
+return getASTContext().getLangOpts();
arphaman wrote:
> kadircet wrote:
> > can we introduce this helper in a new patch, while changing other
> > occurrences in clangd?
> Sounds good, I'll do that.
can you also revert this change?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69543/new/
https://reviews.llvm.org/D69543
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69543: [clangd] Add a tweak refactoring to wrap Objective-C string literals in `NSLocalizedString` macros
arphaman added inline comments.
Comment at: clang-tools-extra/clangd/ParsedAST.h:80
+ const LangOptions &getLangOpts() const {
+return getASTContext().getLangOpts();
kadircet wrote:
> can we introduce this helper in a new patch, while changing other occurrences
> in clangd?
Sounds good, I'll do that.
Comment at:
clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp:34
+/// After:
+/// NSLocalizedString(@"description", "")
+class ObjCLocalizeStringLiteral : public Tweak {
kadircet wrote:
> NSLocalizedString(@"description", `@`"")
Great catch, thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69543/new/
https://reviews.llvm.org/D69543
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69543: [clangd] Add a tweak refactoring to wrap Objective-C string literals in `NSLocalizedString` macros
arphaman updated this revision to Diff 231799.
arphaman marked 6 inline comments as done.
arphaman added a comment.
Fixed nits.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69543/new/
https://reviews.llvm.org/D69543
Files:
clang-tools-extra/clangd/ParsedAST.h
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -122,6 +122,25 @@
EXPECT_EQ(apply(Input), Output);
}
+TWEAK_TEST(ObjCLocalizeStringLiteral);
+TEST_F(ObjCLocalizeStringLiteralTest, Test) {
+ ExtraArgs.push_back("-x");
+ ExtraArgs.push_back("objective-c");
+
+ // Ensure the the action can be initiated in the string literal.
+ EXPECT_AVAILABLE(R"(id x = ^[[@[[^"^t^est^";)");
+
+ // Ensure that the action can't be initiated in other places.
+ EXPECT_UNAVAILABLE(R"([[i^d ^[[x]] ^= @"test";^]])");
+
+ // Ensure that the action is not available for regular C strings.
+ EXPECT_UNAVAILABLE(R"(const char * x= "^test";)");
+
+ const char *Input = R"(id x = [[@"test"]];)";
+ const char *Output = R"(id x = NSLocalizedString(@"test", @"");)";
+ EXPECT_EQ(apply(Input), Output);
+}
+
TWEAK_TEST(DumpAST);
TEST_F(DumpASTTest, Test) {
EXPECT_AVAILABLE("^int f^oo() { re^turn 2 ^+ 2; }");
Index: clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
@@ -0,0 +1,85 @@
+//===--- ObjcLocalizeStringLiteral.cpp ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Logger.h"
+#include "ParsedAST.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ExprObjC.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+/// Wraps an Objective-C string literal with the NSLocalizedString macro.
+/// Before:
+/// @"description"
+/// ^^^
+/// After:
+/// NSLocalizedString(@"description", @"")
+class ObjCLocalizeStringLiteral : public Tweak {
+public:
+ const char *id() const override final;
+ Intent intent() const override { return Intent::Refactor; }
+
+ bool prepare(const Selection &Inputs) override;
+ Expected apply(const Selection &Inputs) override;
+ std::string title() const override;
+
+private:
+ const clang::ObjCStringLiteral *Str = nullptr;
+};
+
+REGISTER_TWEAK(ObjCLocalizeStringLiteral)
+
+bool ObjCLocalizeStringLiteral::prepare(const Selection &Inputs) {
+ const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
+ if (!N)
+return false;
+ // Allow the refactoring even if the user selected only the C string part
+ // of the expression.
+ if (N->ASTNode.get()) {
+if (N->Parent)
+ N = N->Parent;
+ }
+ Str = dyn_cast_or_null(N->ASTNode.get());
+ return Str;
+}
+
+Expected
+ObjCLocalizeStringLiteral::apply(const Selection &Inputs) {
+ auto &SM = Inputs.AST.getSourceManager();
+ auto &LangOpts = Inputs.AST.getASTContext().getLangOpts();
+ auto Reps = tooling::Replacements(tooling::Replacement(
+ SM, CharSourceRange::getCharRange(Str->getBeginLoc()),
+ "NSLocalizedString(", LangOpts));
+ SourceLocation EndLoc = Lexer::getLocForEndOfToken(
+ Str->getEndLoc(), 0, Inputs.AST.getSourceManager(), LangOpts);
+ if (auto Err = Reps.add(tooling::Replacement(
+ SM, CharSourceRange::getCharRange(EndLoc), ", @\"\")", LangOpts)))
+return std::move(Err);
+ return Effect::mainFileEdit(SM, std::move(Reps));
+}
+
+std::string ObjCLocalizeStringLiteral::title() const {
+ return "Wrap in NSLocalizedString";
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===
--- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -19,6 +19,7 @@
ExpandMacro.cpp
ExtractFunction.cpp
ExtractVariable.cpp
+ ObjCLocalizeStringLiteral.cpp
Raw
[PATCH] D69543: [clangd] Add a tweak refactoring to wrap Objective-C string literals in `NSLocalizedString` macros
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69543: [clangd] Add a tweak refactoring to wrap Objective-C string literals in `NSLocalizedString` macros
arphaman updated this revision to Diff 228548.
arphaman marked 2 inline comments as done.
arphaman retitled this revision from "[WIP][clangd] Add a tweak refactoring to
wrap Objective-C string literals in `NSLocalizedString` macros" to "[clangd]
Add a tweak refactoring to wrap Objective-C string literals in
`NSLocalizedString` macros".
arphaman added a comment.
I figured out what I was doing wrong in the tests, and fixed them. This is no
longer WIP and is ready for review.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69543/new/
https://reviews.llvm.org/D69543
Files:
clang-tools-extra/clangd/ParsedAST.h
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -122,6 +122,29 @@
EXPECT_EQ(apply(Input), Output);
}
+TWEAK_TEST(ObjCLocalizeStringLiteral);
+TEST_F(ObjCLocalizeStringLiteralTest, Test) {
+ ExtraArgs.push_back("-x");
+ ExtraArgs.push_back("objective-c");
+
+ // Ensure the the action can be initiated in the string literal.
+ EXPECT_AVAILABLE(R"(id x = ^@^"^t^est^";)");
+ EXPECT_AVAILABLE(R"(id x = [[@"test"]];)");
+ EXPECT_AVAILABLE(R"(id x = @[["test"]];)");
+
+ // Ensure that the action can't be initiated in other places.
+ EXPECT_UNAVAILABLE(R"(i^d ^x ^= @"test";^)");
+ EXPECT_UNAVAILABLE(R"(id [[x]] = @"test";)");
+ EXPECT_UNAVAILABLE(R"([[id x = @"test";]])");
+
+ // Ensure that the action is not available for regular C strings.
+ EXPECT_UNAVAILABLE(R"(const char * x= "^test";)");
+
+ const char *Input = R"(id x = [[@"test"]];)";
+ const char *Output = R"(id x = NSLocalizedString(@"test", @"");)";
+ EXPECT_EQ(apply(Input), Output);
+}
+
TWEAK_TEST(DumpAST);
TEST_F(DumpASTTest, Test) {
EXPECT_AVAILABLE("^int f^oo() { re^turn 2 ^+ 2; }");
Index: clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
@@ -0,0 +1,86 @@
+//===--- ObjcLocalizeStringLiteral.cpp ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Logger.h"
+#include "ParsedAST.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ExprObjC.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+/// Wraps an Objective-C string literal with the NSLocalizedString macro.
+/// Before:
+/// @"description"
+/// ^^^
+/// After:
+/// NSLocalizedString(@"description", "")
+class ObjCLocalizeStringLiteral : public Tweak {
+public:
+ const char *id() const override final;
+ Intent intent() const override { return Intent::Refactor; }
+
+ bool prepare(const Selection &Inputs) override;
+ Expected apply(const Selection &Inputs) override;
+ std::string title() const override;
+
+private:
+ const clang::ObjCStringLiteral *Str = nullptr;
+};
+
+REGISTER_TWEAK(ObjCLocalizeStringLiteral)
+
+bool ObjCLocalizeStringLiteral::prepare(const Selection &Inputs) {
+ const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
+ if (!N)
+return false;
+ // Allow the refactoring even if the user selected only the C string part
+ // of the expression.
+ if (N->ASTNode.get()) {
+if (N->Parent)
+ N = N->Parent;
+ }
+ Str = dyn_cast_or_null(N->ASTNode.get());
+ return Str;
+}
+
+Expected
+ObjCLocalizeStringLiteral::apply(const Selection &Inputs) {
+ auto &SM = Inputs.AST.getSourceManager();
+ auto Reps = tooling::Replacements(tooling::Replacement(
+ SM, CharSourceRange::getCharRange(Str->getBeginLoc()),
+ "NSLocalizedString(", Inputs.AST.getASTContext().getLangOpts()));
+ SourceLocation EndLoc = Lexer::getLocForEndOfToken(
+ Str->getEndLoc(), 0, Inputs.AST.getSourceManager(),
+ Inputs.AST.getLangOpts());
+ if (auto Err = Reps.add(tooling::Replacement(
+ SM, CharSourceRange::getCharRange(EndLoc), ", @\"\")",
+ Inputs.AST.getASTContext().getLangOpts(
+return std::move(Err);
+ return Effect::mainFileE
