search

AcademySoftwareFoundation / openexr

The OpenEXR project provides the specification and reference implementation of the EXR file format, the professional-grade image storage format of the motion picture industry.
http://www.openexr.com/
BSD 3-Clause "New" or "Revised" License
1.62k stars 614 forks source link

Properly document the OpenEXR coding style #394

Closed cary-ilm closed 5 years ago

cary-ilm commented 5 years ago

Will clang-format work? Not sure if it can be made to enforce the existing formatting style.

lgritz commented 5 years ago

I started using it for OIIO and love it.

There are three big costs that are going to be hard to swallow, and maybe you'll decide they are too steep and we don' want to do this for OpenEXR:

(1) You can noodle with it until it's kinda close to the house style, but it's very hard to make it exact (even if somehow your codebase starts out uniform). At some point you need to accept that it will be uniformly 90% your favourite style instead of 100%.

(2) It totally hoses your 'git blame' history because you after the big reformat checkin, you may have massive parts of the code base all look like they were written by the person who did the reformatting, on the date that they did the formatting. Afterwards it will be hard to look at old code and immediately know who wrote that line and when, without more digging.

(3) The reformatting will make a one-time barrier in which it's very hard to cleanly merge after-reformat patches into before-reformat supported branches, because the diffs just won't line up well.

But what do you get for all this?

It's very important to do it immediately before a major release. For example, if you do it right before a 2.4 release, both the 2.4 branch and the new master (2.5 dev) are new formatting, they will be easy to merge patches form one to the other, and you probably will only rarely need to do the more difficult merge back to 2.2 or 2.3 obsolete branches. If you do it an any time other than immediately prior to a new major release, it will create problematic barriers where you can't cherry-pick patches between two branches that you are trying to maintain simultaneously.

What you want is the time machine that would let you start using clang-format on the day you wrote the first line of code, so that you never have one of those one-time massive changes. But you can't do that, so what you must do is decide that you can live with the one-time inconvenience. In a few years, that barrier won't be important to you any more.

I don't regret doing it to OIIO one bit, and I'm looking forward to the next major OSL release so I can do it with that code base as well.

lgritz commented 5 years ago

Two additional notes:

  1. clang-format only autoates your whitespace and line-wrapping formatting rules. That doesn't replace other elements of a style guide for the coding idioms you prefer.

  2. It is possible to mark sections of code, or an entire file, as "have clang-format skip this". Used very sparingly, this can really simplify mental anguish of the task -- there will be time where you know in your heart that a nonstandard formatting of some section make a certain section of the code much much easier to understand than if you let it auto-format.

cary-ilm commented 5 years ago

I suspect the juice isn't worth the squeeze for OpenEXR, given the maturity of the code, although it's worth a little investigation. The documentation should say something about the style accepted, but I'm generally against anything that obscures the edit history.

On Tue, Jun 11, 2019 at 3:35 PM Larry Gritz notifications@github.com wrote:

Two additional notes:

1.

clang-format only autoates your whitespace and line-wrapping formatting rules. That doesn't replace other elements of a style guide for the coding idioms you prefer. 2.

It is possible to mark sections of code, or an entire file, as "have clang-format skip this". Used very sparingly, this can really simplify mental anguish of the task -- there will be time where you know in your heart that a nonstandard formatting of some section make a certain section of the code much much easier to understand than if you let it auto-format.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/openexr/openexr/issues/394?email_source=notifications&email_token=AFC3DGNEMFIH2SYIWZALWWLP2ASBXA5CNFSM4HXDWU7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXOWLLA#issuecomment-501048748, or mute the thread https://github.com/notifications/unsubscribe-auth/AFC3DGJCQDK2JRNVUHP7SY3P2ASBXANCNFSM4HXDWU7A .

-- Cary Phillips | R&D Supervisor | ILM | San Francisco

meshula commented 5 years ago

You can also reformat comments, sort include blocks, and a lot more. Here's an example that covers some of the things it can do. This is close to house style, although I didn't try to figure out how to get a space after a function name and before a parenthesis.

---
AccessModifierOffset: -4
AlignAfterOpenBracket: AlwaysBreak
AlignConsecutiveAssignments: false
AlignConsecutiveDeclarations: false
AlignEscapedNewlinesLeft: true
AlignOperands:   false
AlignTrailingComments: true
AllowAllParametersOfDeclarationOnNextLine: false
AllowShortBlocksOnASingleLine: false
AllowShortCaseLabelsOnASingleLine: true
AllowShortFunctionsOnASingleLine: Empty
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: true
AlwaysBreakTemplateDeclarations: true
BinPackArguments: false
BinPackParameters: false
BraceWrapping:
  AfterClass:      true
  AfterControlStatement: true
  AfterEnum:       true
  AfterFunction:   true
  AfterNamespace:  false
  AfterObjCDeclaration: true
  AfterStruct:     true
  AfterUnion:      true
  BeforeCatch:     true
  BeforeElse:      true
  IndentBraces:    false
BreakBeforeBinaryOperators: None
BreakBeforeBraces: Custom
BreakBeforeTernaryOperators: true
BreakConstructorInitializersBeforeComma: false
BreakAfterJavaFieldAnnotations: false
BreakStringLiterals: false
ColumnLimit:     120
CommentPragmas:  '^ IWYU pragma:'
ConstructorInitializerAllOnOneLineOrOnePerLine: true
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: true
DerivePointerAlignment: false
DisableFormat:   false
FixNamespaceComments: true
ForEachMacros:   [ FOR_EACH_RANGE, FOR_EACH, ]
IncludeBlocks:   Regroup
IncludeCategories:
    # Precompiled headers always first
  - Regex:       '^"stdafx.*"'
    Priority:    -1

    # Local project includes
  - Regex:       '^".*\.h(pp)?"'
    Priority:    3

    # OpenEXR includes
  - Regex:       '^<Ilm\/.*\.h(pp)?>'
    Priority:    5

    # 3rd Party includes
  - Regex:       '^<.*\.h(pp)?>'
    Priority:    10
  - Regex:       '^<.*\/.*>'
    Priority:    10

    # c++ std, then c std
  - Regex:       '^<chrono>'
    Priority:    1000
  - Regex:       '^<c.*>'
    Priority:    1010
  - Regex:       '^<.*>'
    Priority:    1000

    # Everything else
  - Regex:       '.*'
    Priority:    900
IndentCaseLabels: true
IndentPPDirectives: AfterHash
IndentWidth:     4
IndentWrappedFunctionNames: false
KeepEmptyLinesAtTheStartOfBlocks: false
MacroBlockBegin: ''
MacroBlockEnd:   ''
MaxEmptyLinesToKeep: 2
NamespaceIndentation: None
ObjCBlockIndentWidth: 4
ObjCSpaceAfterProperty: false
ObjCSpaceBeforeProtocolList: false
PenaltyBreakBeforeFirstCallParameter: 1
PenaltyBreakComment: 300
PenaltyBreakFirstLessLess: 120
PenaltyBreakString: 1000
PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 200
PointerAlignment: Left
ReflowComments:  true
SortIncludes:    true
SortUsingDeclarations: true
SpaceAfterCStyleCast: false
SpaceAfterTemplateKeyword: false
SpaceBeforeAssignmentOperators: true
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles:  false
SpacesInContainerLiterals: true
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInSquareBrackets: false
Standard:        Cpp11
TabWidth:        4
UseTab:          Never
...
lgritz commented 5 years ago

Doing this to OIIO left me queasy for sure, but 6 months later I'm already considering it a big success. But that doesn't make it a fit for all projects.

Here is my go-to thought for this kind of decision: if transported irrevocably to 2025, what 2019 decision will you be happy (or regretful) about in retrospect?

kdt3rd commented 5 years ago

I would like to argue for clang-format. There are definitely some minor inconsistencies that might be nice to clean up. But the big one is at least some chunk of the files have a mix of tabs / spaces (i.e. assuming tabs are set to 8), which lessens readability across editors, as some people set their tab size to 4 (i.e. that seems to be the default in VS code, unless it read some prefs from somewhere else).

So yes, this initial commit would have a lots of changes. But if we do a decent job matching the rest of the existing coding style w/ the clang format settings, you can then do a git blame -w to skip the whitespace changes, or just look at the change history using gitk / git log / whatever your history browser of choice is. I use git blame very rarely - but I tend to not obsess about who created a bug, and only go looking when I need to ask them what they intended to do...

lgritz commented 5 years ago

I didn't know about git blame -w, thanks for the tip! But surely it's only for whitespace changes within a line and it can't really do it right for lines that clang-format split or joined, right?

To clarify: it's not that I care who created the bug, but I do find it useful to have an immediate answer to "in which version/commit was this line last changed, and who might remember why it was done this way?" Also, "golly, that section of code has been untouched for 8 years through the last 30 films we worked on, so maybe it's not critical enough to push out an emergency release at 5pm on Friday."

Though now that you mention it, as much as I like the rationale I just articulated, I have to admit that in the 7 months since OIIO's big reformat, I don't recall a single actual instance of being frustrated because the dates and ownerships of individual lines had changed. So maybe my caveat about this issue is purely theoretical and unlikely to bother anybody in practice.

cary-ilm commented 5 years ago

I'm increasingly thinking that there are several rather independent collections of code here that have been conflated from the outset:

  1. Imath's vector/matrix classes, possibly combined with the python/numpy bindings currently in PyIlmBase.
  2. The half type, which needs to be rectified somehow with CUDA's half.
  3. Other stuff in IlmBase, such as IlmThread
  4. OpenEXR proper.

If we split the Imath vector/matrix classes into a separate project and promote it as an ASWF-annointed thing, it will need proper documentation, which it doesn't currently have, presumably via doxygen, which will mean touching a lot of the header files at least. This would seem a good time to take a wholesale reformatting hit.

On Wed, Jun 12, 2019 at 8:31 AM Larry Gritz notifications@github.com wrote:

I didn't know about git blame -w, thanks for the tip! But surely it's only for whitespace changes within a line and it can't really do it right for lines that clang-format split or joined, right?

To clarify: it's not that I care who created the bug, but I do find it useful to have an immediate answer to "in which version/commit was this line last changed, and who might remember why it was done this way?" Also, "golly, that section of code has been untouched for 8 years through the last 30 films we worked on, so maybe it's not critical enough to push out an emergency release at 5pm on Friday."

Though now that you mention it, as much as I like the rationale I just articulated, I have to admit that in the 7 months since OIIO's big reformat, I don't recall a single actual instance of being frustrated because the dates and ownerships of individual lines had changed. So maybe my caveat about this issue is purely theoretical and unlikely to bother anybody in practice.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/openexr/openexr/issues/394?email_source=notifications&email_token=AFC3DGKENCO35U52ZBYBBD3P2EJDLA5CNFSM4HXDWU7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXQ2KUI#issuecomment-501327185, or mute the thread https://github.com/notifications/unsubscribe-auth/AFC3DGJ4PNLSUMNQVZWWB6DP2EJDLANCNFSM4HXDWU7A .

-- Cary Phillips | R&D Supervisor | ILM | San Francisco

cary-ilm commented 5 years ago

The current draft is good enough for now, until we investigate clang-format further.