llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.11k stars 12.01k forks source link

r233304 Regression: All ObjC field access lands on the new line #23598

Open llvmbot opened 9 years ago

llvmbot commented 9 years ago
Bugzilla Link 23224
Version trunk
OS All
Reporter LLVM Bugzilla Contributor

Extended Description

The commit to blame is r233304

clang-format: Force line break in trailing calls after multline exprs.

Actually it forces line break on any dot access not only calls, which causes severe code style regressions in our ObjC project.

Also I think such change should have appropriate format setting rather than making it default.

Some regression examples:

   CGFloat height =
       [cell.contentView
-          systemLayoutSizeFittingSize:UILayoutFittingCompressedSize].height;
+          systemLayoutSizeFittingSize:UILayoutFittingCompressedSize]
+          .height;

-  NSDate *fileDate = [[NSFileManager defaultManager]
-                         attributesOfItemAtPath:jsonPath
-                                          error:NULL].fileModificationDate;
+  NSDate *fileDate =
+      [[NSFileManager defaultManager] attributesOfItemAtPath:jsonPath
+                                                       error:NULL]
+          .fileModificationDate;

-  NSDate *fileDate = [[NSFileManager defaultManager]
-                         attributesOfItemAtPath:pathForDefinitions
-                                          error:NULL].fileModificationDate;
+  NSDate *fileDate =
+      [[NSFileManager defaultManager] attributesOfItemAtPath:pathForDefinitions
+                                                       error:NULL]
+          .fileModificationDate;

   self.layer.borderColor = [UIColor colorWithHue:hue
                                       saturation:saturation
                                       brightness:brightness * .8
-                                           alpha:alpha].CGColor;
+                                           alpha:alpha]
+
llvmbot commented 9 years ago

Now, I am not an ObjC programmer, so I don't know, but things like (...)

Unfortunately I am almost everyday ObjC programmer, and really haven't seen anyone breaking before .dotAccess, just because:

obj.property

has exactly same meaning (and it is simple syntax sugar for:

[obj property]

So following code:

CGFloat height = [cell.contentView systemLayoutSizeFittingSize:UILayoutFittingCompressedSize] .height;

Has exactly the same meaning that:

CGFloat height = [[cell.contentView systemLayoutSizeFittingSize:UILayoutFittingCompressedSize] height];

Actually dot notation was introduced rather recently (ObjC v2.0), to express calls that serve as property reads. But technically these are same calls regardless of notation.

Still as you can see in example above clang-format gives them completely different formatting in these two cases.

What's worse the clang-format r233304 change was described as improving builder type calls formatting. But samples above are barely builder type calls. Dot access purpose is to express exactly opposite - a property call, not builder type call.

So if you want to be consequent we should have, when using non dot notation:

CGFloat height = [[cell.contentView systemLayoutSizeFittingSize:UILayoutFittingCompressedSize] height];

Same applies to:

NSDate *fileDate = [[NSFileManager defaultManager] attributesOfItemAtPath:jsonPath error:NULL] .fileModificationDate;

which looks very very awkward to me as ObjC programmer.

Therefore ideally it would be to have some setting where I can simply turn it off, language type call basis preferred, and off by default for ObjC calls, on for C/C++ calls.

llvmbot commented 9 years ago

All those changes are intended. The commit message is wrong. Should have been "trailing member accesses" or something.

Now, I am not an ObjC programmer, so I don't know, but things like

  NSDate *fileDate = [[NSFileManager defaultManager]
                         attributesOfItemAtPath:pathForDefinitions
                                          error:NULL].fileModificationDate;

Is exactly what it is meant to prevent for C++ calls. There is almost no visible structure and the statements gets very hard to understand.