This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Do not break Objective-C string literals inside array literals
ClosedPublic

Authored by benhamilton on Jan 30 2018, 12:42 PM.

Details

Summary

Concatenating Objective-C string literals inside an array literal
raises the warning -Wobjc-string-concatenation (which is enabled by default).

clang-format currently splits and concatenates string literals like
the following:

NSArray *myArray = @[ @"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" ];

into:

NSArray *myArray =
      @[ @"aaaaaaaaaaaaaaaaaaaaaaaaaaaa"
         @"aaaaaaaaa" ];

which raises the warning. This is https://bugs.llvm.org/show_bug.cgi?id=36153 .

The options I can think of to fix this are:

  1. Have clang-format disable Wobjc-string-concatenation by emitting

pragmas around the formatted code

  1. Have clang-format wrap the string literals in a macro (which

disables the warning)

  1. Disable string splitting for Objective-C string literals inside

array literals

I think 1) has no precedent, and I couldn't find a good
identity() macro for 2). So, this diff implements 3).

Test Plan: make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests

Diff Detail

Repository
rC Clang

Event Timeline

benhamilton created this revision.Jan 30 2018, 12:42 PM

Ping! Any comments?

The summary states that -Wobjc-string-concatenation is enabled by default? I looked through include/clang/Basic/DiagnosticGroups.td and did not see evidence that ObjCStringConcatenation is under All. Am I missing something? Is -Wobjc-string-concatenation enabled by default through some other mechanism?

@stephanemoore: It appears all warnings are enabled by default unless they are in class DefaultIgnore:

https://github.com/llvm-mirror/clang/blob/6de2efd1953adaa9a190b2cdfbe7b5c15f6d6efe/include/clang/Basic/Diagnostic.td#L105

This diagnostic is not in DefaultIgnore:

https://github.com/llvm-mirror/clang/blob/a58d0437d1e17d53d7bffea598d77789dd6d28b6/include/clang/Basic/DiagnosticGroups.td#L933

In fact, the All class is specifically for warnings which *are* in DefaultIgnore but which should be emitted with -Wall:

https://github.com/llvm-mirror/clang/blob/a58d0437d1e17d53d7bffea598d77789dd6d28b6/include/clang/Basic/DiagnosticGroups.td#L782

// Note that putting warnings in -Wall will not disable them by default. If a
// warning should be active _only_ when -Wall is passed in, mark it as
// DefaultIgnore in addition to putting it here.
jolesiak accepted this revision.Feb 8 2018, 1:49 AM
This revision is now accepted and ready to land.Feb 8 2018, 1:49 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the explanation 👌 I'm still learning the inner workings of clang 😅

The change looks good to me.