Apply standard coding style by theiostream · Pull Request #2 · gnustep/libs-opal

theiostream · 2017-07-10T21:55:52Z

As described in the commit message, apply a standard code style for Opal with the following configurations:

Use broken braces without GNU-indented blocks
Use GNU-style indentation (two spaces as tabs)
Use GNU-style Objective-C:
- Insert GNU padding after Objective-C method colon
- Align Objective-C colons in declarations and calls
Indent cases inside switch blocks
Pad parentheses after statements (e.g. if () as opposed to if())
Insert one indent for continuing a function call.
Do not limit line width.

Unlike other GNUstep projects (Base, GUI, CoreBase), which may not conform fully to a coding style but mostly converge to one, Opal is currently a very big mess when it comes to styles to the point that reading code becomes really hard sometimes. This one I came up with tries to resemble most of the codebase.

If you agree with applying this, I intend on adding a CI build stage which fails the build if AStyle detects a file is outside these constraints.

On a sidenote, I am sure either formatter bugs or a bad config may have generated awkward or worse code in parts of the codebase. But the overall

For purposes of not corrupting the git history with this huge commit, Git provides git blame -w, which ignores whitespaces from a blame for debugging purposes.

This change is

theiostream · 2017-07-10T22:05:36Z

@ivucica, I can't request a review in the GitHub UI so I think this mention is a way of doing it.

(I do not expect this to be merged immediately or as-is, or ever; it's just a discussion-prompter).

ivucica

While I like consistency, I am not a huge fan of reformat changes just for the sake of reformatting unless there is a clear benefit or clean standard. The very size of the change (over 3000LOC) says there is no existing expectation of consistency, so while new code should always be consistent, there isn't great benefit in patching existing code.

Having said that -- I don't object greatly either (and as I said I do value consistency). Therefore, I would like to rope in @ericwa as one of the principal authors of Opal to decide.

ivucica · 2017-07-10T22:28:11Z

-                         : (int[]) advances
+: (size_t)count
+: (int[]) advances
 {


This new indentation doesn't seem right and even the fix is not consistent:

: (p)q : (a)b : (c) d

There additional cases like this one.

This is a formatter bug. I'll report it.

ivucica · 2017-07-10T22:30:45Z

  }
-  if (t1.b != t2.b) {
+  if (t1.b != t2.b)
+  {


GNU style, the way I understand it, would require two additional spaces before the brace. For example:

if (t1.b != t2.b) { return false; } ```

Agreed, I would favour doing the GNU style brace indentation.

ivucica · 2017-07-10T22:31:52Z

      }
      else if (nameStruct.platform_id == TT_PLATFORM_MICROSOFT &&
-               nameStruct.encoding_id == TT_MS_ID_UNICODE_CS)
+        nameStruct.encoding_id == TT_MS_ID_UNICODE_CS)


I believe this fix is wrong, and the original indentation should be correct. I don't have GNU style guide at hand, so I may be wrong.

There are multiple instances of this.

I'd agree with @ivucica : for multiline expressions the lines after the first one shouldn't be indented left of the opening paren.

Here's an example in libs-gui:
https://github.com/gnustep/libs-gui/blob/master/Source/NSTextView.m#L1677

This was another attempt to mimic the prevalent "Opal style" rather than canonical GNU style. This is easily changeable in the formatter config.

ivucica · 2017-07-10T22:32:39Z

-    {
-      fprintf(stderr,"Cannot open display: %s\n", XDisplayName(NULL));
-      exit(EXIT_FAILURE);
-    }


The original indentation here is an example of the correct indentation for GNUstep.

ericwa · 2017-07-10T23:51:36Z

Hi! Sorry for leaving things in a messy formatting state!

I am OK with merging a reformat, but I would suggest aiming for the style used by CoreBase / libs-back / libs-gui, rather than trying to preserve elements of the original Opal style.

One question is whether to use tab characters at all; it seems older code in GNUstep does but some newer code doesn't (corebase, e.g. the cairo backend in libs-back). I don't see it mentioned explicitly in https://www.gnu.org/prep/standards/standards.html#Formatting , but the manpage for GNU indent says the default is to use tabs (-ut option). Using tabs with GNU brace indentation always seemed odd to me, since they have to be rendered as 8 spaces or the formatting gets completely ruined.

Also - I noticed CGGradient.h is showing ^M characters added when I do git diff master theiostream/cleanup, so it looks like it was converted to DOS line endings by accident. Probably all files should be using unix line endings?

Apply a standard coding style for the Opal project, which suffered from terribly deviating styles in a relatively small project. All files were formatted with: > astyle --style=gnu -s2 -xM -S -xP2 -H -xU -xC79 These options preserve the GNU coding standards.

theiostream · 2017-07-11T01:44:07Z

I just updated the pull request to contain a reformat which follows the GNU format (or should, at least). There is also a second commit that has the files converted from DOS line endings to Unix.

Maybe now we have a better reformat to start discussing.

(This time we use astyle --style=gnu -s2 -xM -S -xP2 -H -xC79)

Whenever I try using GNU indent Objective-C stuff gets screwed up. That's why I tend to prefer astyle.

ericwa · 2017-07-11T18:15:31Z


-void CGContextSetGrayStrokeColor(CGContextRef ctx, CGFloat gray, CGFloat alpha);
+void CGContextSetGrayStrokeColor(CGContextRef ctx, CGFloat gray,
+                                 CGFloat alpha);


This looks odd, should be 1 argument per line if it won't all fit on one line?

ericwa · 2017-07-11T18:18:37Z

  CGRect bboxes[])
 {
-  return [font getGlyphBBoxes: glyphs : count : bboxes];
+  return [font getGlyphBBoxes: glyphs: count: bboxes];


whoops, formatter got confused by the weird getGlyphBBoxes::: selector name.

ericwa · 2017-07-11T18:25:37Z

+ (NSArray*) destinationClasses;
+ (Class) destinationClassForType: (NSString*)type;
+
+ (NSArray *)typeIdentifiers;


The formatter doesn't seem to standardize the space before the *:

+ (NSArray*) destinationClasses; + (NSArray *)typeIdentifiers;

I believe (NSArray *) is the standard format in GNUstep, we could probably do a regex fix if the formatter can't fix this.

ericwa · 2017-07-11T18:32:22Z

 /** Returns the rectangle obtained by translating rect
 * horizontally by dx and vertically by dy. */
-OP_GEOM_SCOPE CGRect CGRectOffset(CGRect rect, CGFloat dx, CGFloat dy) OP_GEOM_ATTR;
+OP_GEOM_SCOPE CGRect CGRectOffset(CGRect rect, CGFloat dx,


I just checked libs-gui a bit and the code will go over 80 characers/line when needed to avoid awkward line breaks.
Maybe we should try 100 character columns?

ivucica · 2017-07-23T23:18:12Z

@theiostream Any updates in this PR?

ivucica requested changes Jul 10, 2017

View reviewed changes

theiostream force-pushed the cleanup branch from 61e1d8a to f9b5c7a Compare July 11, 2017 01:40

chore: convert from DOS encoding to Unix

b6a19f0

ericwa reviewed Jul 11, 2017

View reviewed changes

Conversation

theiostream commented Jul 10, 2017 • edited by ivucica Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theiostream commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivucica left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericwa commented Jul 10, 2017

Uh oh!

theiostream commented Jul 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivucica commented Jul 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

theiostream commented Jul 10, 2017 •

edited by ivucica

Loading

theiostream commented Jul 10, 2017 •

edited

Loading