Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial clang-format auto cleanup #2324

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nschonni
Copy link
Collaborator

@nschonni nschonni commented Feb 8, 2017

Not really tuned, just opening for discussion. Just brew install clang-format and then ls **/*.cpp | xargs clang-format -i -style=file with the changes to the config to test stuff out.
Used the main defaults from https://clangformat.com/ with a few minor changes to indenting and spacing to minimize some of the diff.

The second part of this should be to add the CI component from https://github.com/citra-emu/citra/blob/6e5e5be/.travis-build.sh that @am11 pointed out

@xzyfer
Copy link
Contributor

xzyfer commented Feb 8, 2017

Considering this is causing compilation failures, it's clear this isn't a simple noop like a JS linter would be.

I have very mixed feelings, I'm all for some kind of new formatting, but I've been burnt over and over again by these massive commits.

@xzyfer
Copy link
Contributor

xzyfer commented Feb 8, 2017

I'd rather incrementally formatting files as we touch them rather than a big bang commit. My suggestion would be to commit the configuration, and for us to configure our editors to apply the formatting as we work.

@nschonni
Copy link
Collaborator Author

nschonni commented Feb 8, 2017

Yeah, this was more to discuss the settings, then it can be layered in. I'll go back and see about the include re-ordering since i think that might be a chunk of the failures

@nschonni
Copy link
Collaborator Author

nschonni commented Feb 8, 2017

Yup, turning off the SortIncludes and re-running allowed me to build locally again

@am11
Copy link
Contributor

am11 commented Feb 8, 2017

This is great! I actually started prototyping it that day and so far achieved https://gist.github.com/am11/39a65404651f310966d646932c623dd6. I was using clang-format which comes packed with Emscripten (installed few weeks ago; was experimenting WebAssembly version of node-sass with binaryen wasm-sass .. which is at least theoretically very possible goal to achieve 😜).

Two things that I found are:

  • The latest version of clang-format (which usually is not available on many Unices OOTB) has most features / options, so the help from google/stackoverflow is bit challenging as we need to concentrate on which version the OP is using
  • It is hard to deduce the LibSass code formatting style, as in some places, we have custom styles .. which is tricky (if not impossible) to configure in clang-format

I like your approach @nschonni to obtain latest, greatest clang-format via npm pipeline. 👍

@xzyfer
Copy link
Contributor

xzyfer commented Feb 8, 2017 via email

@nschonni
Copy link
Collaborator Author

nschonni commented Feb 8, 2017

The travis failure looks like it's just a timeout, but I can't reset it.
@am11 was there stuff from your clang-format that you want to add to this one? This was essentially just the vanilla setup with minor changes to the sort and pointer character sticking to minimize the diff.
I'm happy to treat this PR as a WIP to just discuss the format changes, but I found that I couldn't layer on the options like you can in ESLint, since if you don't set options, it uses defaults for everything.

@nschonni
Copy link
Collaborator Author

nschonni commented Feb 8, 2017

The other option is to adopt on of the company cofigs (style=? LLVM, Google, Chromium, Mozilla, WebKit) and do a big-bangish conversion.

@am11
Copy link
Contributor

am11 commented Feb 9, 2017

@xzyfer, hool! 😻 I will focus on something else then 😜

@nschonni, my effort was barely a start of beginning; I was trying to adjust the options so it matches majority of our existing codebase style. I started with ast.cpp, but some styles are challenging to keep, like:

    if ((has_ns_ == r.has_ns_) ||
        (has_ns_ && ns_.empty()) ||
        (r.has_ns_ && r.ns_.empty())
    ) {
      if (ns_.empty() && r.ns() == "*") return false;
      else if (r.ns().empty() && ns() == "*") return false;
      else return ns() == r.ns();
    }

With couple of tweaks, the multiline if-condition was getting flattened and single line if-statement was getting braces but couldn't keep the ditto styles.

Wonder if it is possible for such linters+autoformatters to provide "auto-detect style" feature as well (based on the majority of style in code)? Analyze style and emit .clang-format .. that would be a nice feature.

AlignEscapedNewlinesLeft: true
AlignTrailingComments: false
AllowAllParametersOfDeclarationOnNextLine: false
AllowShortFunctionsOnASingleLine: false
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this should be Empty to remove the splitting of {}

@mgreter
Copy link
Contributor

mgreter commented Feb 18, 2017

Just my 2 cents: I'm not a fan of autmated code formatting. IMO humans can format code better than machines to point out the relevant bits. None the less I agree that we should stick to a certain code style, but I personally don't believe it should be too strict. I personally like to work with smart tabs and linux kernel styles in general, but I sometimes also vary this if it makes sense in the given context (ie. putting a block opener on a new line if that block carries important lexical variables).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants