Page MenuHomePhabricator

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

Authored by arphaman on Oct 28 2019, 6:27 PM.

Details

Summary

This is a patch to add a refactoring to Clangd that mimics the existing refactoring action in Xcode that wraps around an Objective-C string literal in an NSLocalizedString macro.

Diff Detail

Event Timeline

arphaman created this revision.Oct 28 2019, 6:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 6:27 PM
kadircet added inline comments.Oct 29 2019, 3:13 AM
clang-tools-extra/clangd/unittests/TweakTests.cpp
127
129

maybe use raw string literals?

R"(@"teststring")"
arphaman updated this revision to Diff 228548.Nov 8 2019, 5:23 PM
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.

I figured out what I was doing wrong in the tests, and fixed them. This is no longer WIP and is ready for review.

arphaman edited the summary of this revision. (Show Details)Nov 8 2019, 5:24 PM

implementation lgtm with a few nits.

main concern is about the new getlangopts helper

clang-tools-extra/clangd/ParsedAST.h
80 ↗(On Diff #228548)

can we introduce this helper in a new patch, while changing other occurrences in clangd?

clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
35

NSLocalizedString(@"description", @"")

70

maybe extract Inputs.AST.getASTContext().getLangOpts() into a variable and make use of it in the following places as well? (line 72 and 75)

clang-tools-extra/clangd/unittests/TweakTests.cpp
131

nit: you can combine all of this into a single test

136

nit: you can combine all of this into a single test

arphaman updated this revision to Diff 231799.Mon, Dec 2, 4:48 PM
arphaman marked 6 inline comments as done.

Fixed nits.

clang-tools-extra/clangd/ParsedAST.h
80 ↗(On Diff #228548)

Sounds good, I'll do that.

clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp
35

Great catch, thanks!

kadircet accepted this revision.Tue, Dec 3, 10:38 AM

Thanks, LGTM!

clang-tools-extra/clangd/ParsedAST.h
80 ↗(On Diff #228548)

can you also revert this change?

This revision is now accepted and ready to land.Tue, Dec 3, 10:38 AM
arphaman marked 2 inline comments as done.Wed, Dec 4, 4:56 PM
arphaman added inline comments.
clang-tools-extra/clangd/ParsedAST.h
80 ↗(On Diff #228548)
This revision was automatically updated to reflect the committed changes.
arphaman marked an inline comment as done.