[clang-format] always use a line per constructor initialiser for longer lines #16962

phunkyfish · 2019-11-24T20:09:57Z

Description

Ensures for longer lines that this occurs:

MyClass::CMyClass(bool bBoolArg,
                  int iIntegerArg,
                  const std::string& strSomeText,
                  const std::shared_ptr<CMyOtherClass>& myOtherClass)
  : m_bBoolArg(bBoolArg),
    m_iIntegerArg(iIntegerArg),
    m_strSomeText(strSomeText),
    m_myOtherClass(myOtherClass)
{
}

But never this:

MyClass::CMyClass(bool bBoolArg,
                  int iIntegerArg,
                  const std::string& strSomeText,
                  const std::shared_ptr<CMyOtherClass>& myOtherClass)
  : m_bBoolArg(bBoolArg), m_iIntegerArg(iIntegerArg), m_strSomeText(strSomeText)
{
}

I.e. if a constructor lines is longer than .clang-format column limit then there will always be one initialiser per line.

This change does not effect constructor lines short than the .clang-format column limit.

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

Paxxi

I'm a bit worried that using such a new option could lead to a lot of churn on constructor formatting depending on the version of clang-format.
Either we accept that and go ahead or we don't use it. If someone has submitted a clang-formatted PR we should never request formatting changes because they used the wrong clang-format version imo.

ksooo · 2019-11-25T06:14:46Z

If someone has submitted a clang-formatted PR we should never request formatting changes because they used the wrong clang-format version imo.

Why not? We also require them to use other tools with at least a certain version. I don't see a difference to minimal required SDK version, for example.

Paxxi · 2019-11-25T06:19:32Z

Linux distros are slow to add newer versions so it could make it a hassle to install the correct version. Is v9 available in any version of Ubuntu right now?

One of the main values with clang-format imo is that it lowers the bar to contribute. If you use the tool your PR should pass formatting guidelines.

Rechi

The documentation mentioning clang-format has to be updated that at least version 9 is required.
The server running git-clang-format for the whole PR and per commit during a PR build has to be updated.

Paxxi · 2019-11-25T07:44:25Z

Looking at https://packages.ubuntu.com/search?suite=default&section=all&arch=any&keywords=clang-format&searchon=names it appears that clang-format 9 isn't available in Ubuntu yet. If that's correct it's not feasible to require that they install or have it installed already unless we provide it in our apt repo.

fritsch · 2019-11-25T08:29:10Z

It's here: https://packages.ubuntu.com/search?suite=default&section=all&arch=any&keywords=clang-format-9&searchon=names but only in eon

Paxxi · 2019-11-25T08:52:13Z

Oh, I read the (devel) as it not being released yet, sorry about that 😄

fritsch · 2019-11-25T10:39:10Z

You are right never the less. Many are on 18.04 LTS and will stay there until 20.04

phunkyfish · 2019-11-25T10:42:59Z

If you run clang-format using the above .clang-format but are using clang-format 8 does it give an error or ignore the option?

I.e. can anyone using clang-format 8 check this?

phunkyfish · 2020-03-09T18:15:58Z

As 20.04 is out soon maybe this can be revisited soon.

phunkyfish · 2020-04-19T11:43:06Z

Ubuntu should be ok now:

Package clang-format-9
bionic (18.04LTS) (devel): Tool to format C/C++/Obj-C code [universe]
1:9-2~ubuntu18.04.2 [security]: amd64 i386
bionic-updates (devel): Tool to format C/C++/Obj-C code [universe]
1:9-2~ubuntu18.04.2: amd64 arm64 armhf i386 ppc64el s390x
eoan (19.10) (devel): Tool to format C/C++/Obj-C code [universe]
1:9-2: amd64 arm64 armhf i386 ppc64el s390x
focal (devel): Tool to format C/C++/Obj-C code [universe]
1:9.0.1-12: amd64 arm64 armhf i386 ppc64el s390x

@Rechi where and how would I update this on this on the server running git-clang-format for PRs?

Rechi · 2020-04-19T11:47:23Z

You are missing Ubuntu Xenial and also the correct package for checking versions is clang-format instead of clang-format-.

phunkyfish · 2020-04-19T12:37:18Z

You are missing Ubuntu Xenial and also the correct package for checking versions is clang-format instead of clang-format-.

Ok, then maybe this waits for kodi 20. I don't follow the mention of clang-format-. Did I miss something?

DaveTBlake · 2021-02-07T19:01:29Z

This seems to have got forgotten, and clearly isn't happening in v19 code, so bump to v20 and @phunkyfish please pursue it for that.

phunkyfish · 2021-02-07T19:02:20Z

Will do. Thanks @DaveTBlake

phunkyfish · 2022-10-06T21:13:15Z

Jenkins build this please

fuzzard · 2022-10-07T01:37:22Z

cmake/modules/buildtools/FindClangFormat.cmake

+execute_process(COMMAND clang-format --version ERROR_QUIET OUTPUT_VARIABLE CLANG_FORMAT_VERSION_OUTPUT)
+string(REGEX MATCH "([0-9]+)\\..*" CLANG_FORMAT_VERSION ${CLANG_FORMAT_VERSION_OUTPUT})
+if(CLANG_FORMAT_VERSION VERSION_LESS_EQUAL "9.0.0")
+    message(WARNING "clang-format must be at least version 9.0 and the version installed is ${CLANG_FORMAT_VERSION}")
+    set(CLANG_FORMAT_FOUND OFF)
+endif()
+


Can i suggest the following

#.rst: # FindClangFormat # ---------- # Finds clang-format find_program(CLANG_FORMAT_EXECUTABLE clang-format) include(FindPackageHandleStandardArgs) find_package_handle_standard_args(ClangFormat REQUIRED_VARS CLANG_FORMAT_EXECUTABLE) if(CLANGFORMAT_FOUND) set(CLANGFORMAT_MINVERSION "9.0") execute_process(COMMAND clang-format --version ERROR_QUIET OUTPUT_VARIABLE CLANG_FORMAT_VERSION_OUTPUT) string(REGEX MATCH "([0-9]+)\\..*" CLANG_FORMAT_VERSION ${CLANG_FORMAT_VERSION_OUTPUT}) if(CLANG_FORMAT_VERSION VERSION_LESS_EQUAL ${CLANGFORMAT_MINVERSION}) message(WARNING "clang-format must be at least version ${CLANGFORMAT_MINVERSION}. The version found is ${CLANG_FORMAT_VERSION}") set(CLANGFORMAT_FOUND OFF CACHE BOOL "" FORCE) set(CLANG_FORMAT_EXECUTABLE "CLANG_FORMAT_EXECUTABLE-NOTFOUND" CACHE STRING "" FORCE) endif() endif() mark_as_advanced(CLANG_FORMAT_EXECUTABLE)

We only do the version check if clang-format is found. We extract the version number to s single variable (for if/when it needs to be updated).
It also sets the cache variable for CLANGFORMAT_FOUND and CLANG_FORMAT_EXECUTABLE to false/notfound

Rechi

Version hacking in the find module is wrong.
Use the correct CMake way for checking versions.

fuzzard · 2022-10-07T07:12:39Z

I dont really understand what you're asking for here @Rechi

find_program calls do not have any understanding of versioning, and do not make version info available.
Clang-format doesnt provide *ConfigVersion.cmake files to retrieve any version info, there is no pkg-config data provided either.

What exactly is the "Correct Cmake way" for this?

…er lines

Rechi · 2022-10-16T07:45:08Z

Read the documentation of find_package and FindPackageHandleStandardArgs and how those two commands interact. There is no need to touch the find module.

There is also one dependency of Kodi with a minimum version requirement, where it is done correct.

phunkyfish · 2022-10-16T08:24:06Z

Which dependency?

phunkyfish mentioned this pull request Nov 24, 2019

[clang-format] change format for constructor init and switch/case indent #16701

Merged

13 tasks

phunkyfish requested review from Rechi, yol, AlwinEsch, Paxxi, garbear, ksooo, DaveTBlake, fritsch, notspiff and lrusak November 24, 2019 20:10

phunkyfish added v19 Matrix Type: Improvement non-breaking change which improves existing functionality labels Nov 24, 2019

garbear approved these changes Nov 25, 2019

View reviewed changes

Paxxi approved these changes Nov 25, 2019

View reviewed changes

Rechi reviewed Nov 25, 2019

View reviewed changes

phunkyfish added the PR Cleanup: Merge Candidate Candidate for merging after PR cleanup label Mar 9, 2020

AlwinEsch approved these changes Mar 9, 2020

View reviewed changes

phunkyfish force-pushed the clang-format-init branch from cc5d024 to e254231 Compare April 19, 2020 11:47

DaveTBlake added this to the N* 20.0 Alpha 1 milestone Feb 7, 2021

fuzzard modified the milestones: Nexus 20.0 Alpha 1, Nexus 20.0 Alpha 2 May 15, 2022

fuzzard modified the milestones: Nexus 20.0 Alpha 2, Nexus 20.0 Alpha 3 Jul 8, 2022

fuzzard modified the milestones: Nexus 20.0 Alpha 3, O* 21.0 Alpha 1 Sep 8, 2022

fuzzard added v21 Omega and removed v20 Nexus labels Sep 8, 2022

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 4, 2022

phunkyfish force-pushed the clang-format-init branch 2 times, most recently from f5a2f8d to 8ce846c Compare October 4, 2022 21:32

Remove trailing whotespace from Code Guidelines

7c98c99

phunkyfish force-pushed the clang-format-init branch 2 times, most recently from 05d98d4 to be7fb43 Compare October 4, 2022 21:43

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 4, 2022

fuzzard reviewed Oct 7, 2022

View reviewed changes

Rechi requested changes Oct 7, 2022

View reviewed changes

[clang-format] always use a line per constructor initialiser for long…

a5a0a5d

…er lines

phunkyfish force-pushed the clang-format-init branch from be7fb43 to a5a0a5d Compare October 7, 2022 08:50

fuzzard modified the milestones: Omega 21.0 Alpha 1, Omega 21.0 Alpha 2 Apr 14, 2023

fuzzard removed this from the Omega 21.0 Alpha 2 milestone May 10, 2023

fuzzard removed the v21 Omega label May 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-format] always use a line per constructor initialiser for longer lines #16962

[clang-format] always use a line per constructor initialiser for longer lines #16962

phunkyfish commented Nov 24, 2019

Paxxi left a comment

ksooo commented Nov 25, 2019

Paxxi commented Nov 25, 2019

Rechi left a comment

Paxxi commented Nov 25, 2019

fritsch commented Nov 25, 2019

Paxxi commented Nov 25, 2019

fritsch commented Nov 25, 2019

phunkyfish commented Nov 25, 2019 •

edited

phunkyfish commented Mar 9, 2020

phunkyfish commented Apr 19, 2020

Rechi commented Apr 19, 2020

phunkyfish commented Apr 19, 2020

DaveTBlake commented Feb 7, 2021

phunkyfish commented Feb 7, 2021

phunkyfish commented Oct 6, 2022

fuzzard Oct 7, 2022

Rechi left a comment

fuzzard commented Oct 7, 2022

Rechi commented Oct 16, 2022

phunkyfish commented Oct 16, 2022

[clang-format] always use a line per constructor initialiser for longer lines #16962

Are you sure you want to change the base?

[clang-format] always use a line per constructor initialiser for longer lines #16962

Conversation

phunkyfish commented Nov 24, 2019

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Checklist:

Paxxi left a comment

Choose a reason for hiding this comment

ksooo commented Nov 25, 2019

Paxxi commented Nov 25, 2019

Rechi left a comment

Choose a reason for hiding this comment

Paxxi commented Nov 25, 2019

fritsch commented Nov 25, 2019

Paxxi commented Nov 25, 2019

fritsch commented Nov 25, 2019

phunkyfish commented Nov 25, 2019 • edited

phunkyfish commented Mar 9, 2020

phunkyfish commented Apr 19, 2020

Rechi commented Apr 19, 2020

phunkyfish commented Apr 19, 2020

DaveTBlake commented Feb 7, 2021

phunkyfish commented Feb 7, 2021

phunkyfish commented Oct 6, 2022

fuzzard Oct 7, 2022

Choose a reason for hiding this comment

Rechi left a comment

Choose a reason for hiding this comment

fuzzard commented Oct 7, 2022

Rechi commented Oct 16, 2022

phunkyfish commented Oct 16, 2022

phunkyfish commented Nov 25, 2019 •

edited