.clang-format: Set column limit to 80 #2486

Holzhaus · 2020-02-04T23:41:34Z

Right now, running clang-format or git clang-format will unwrap lines
and make them longer than 80 chars, even if the developer inserted
linebreaks manually.

Right now, running clang-format or git clang-format will unwrap lines and make them longer than 80 chars, even if the developer inserted linebreaks manually.

uklotzde · 2020-02-04T23:57:33Z

We had long discussions if we should define a limit or not. As a workaround line breaks can be preserved by placing arguments on separate lines.

If we decide to set a limit the comment becomes obsolete and should be removed.

daschuer · 2020-02-05T16:01:11Z

I am fine with the current solution, allow lines until scrolling with GitHub.

Manually break < 80 in a way clang does not unbreak it.

Unfortunately clang format leaks two independent parameters for this.

Holzhaus · 2020-02-05T16:42:47Z

Right now, the situation is really bad, because I have code that fits our coding style and running clang-format breaks it. This is probably the worst case scenario for an auto-formatter.

I don't really know what's the best way forward, but I see 4 options here:

We set the clang-format column limit to 80/100/120/whatever
We remove the "break long lines" rule from our code style and don't tell people to break long lines in GitHub reviews anymore
We remove clang-format from the pre-commit config and stop recommending people to run it
We find another C++ formatter that keeps (possibily superfluous) manual line breaks

daschuer · 2020-02-05T17:13:59Z

Please point me to the code. There should be a way not to break our coding style and clang-format is happy.

Holzhaus · 2020-02-05T18:10:49Z

Exhibit A

https://github.com/mixxxdj/mixxx/pull/2480/files#diff-8cdef3b229490e40d04ea2c72abe3d62R346-R347

Diff after running clang-format:

-QString parseCrate(const QSqlDatabase& database, QString databasePath,
-                   QString crateFilePath, const QMap<QString, int>& trackIdMap) {
+QString parseCrate(const QSqlDatabase& database, QString databasePath, QString crateFilePath, const QMap<QString, int>& trackIdMap) {

Exhibit B

https://github.com/mixxxdj/mixxx/pull/2480/files#diff-8cdef3b229490e40d04ea2c72abe3d62R486-R501

-            "INSERT INTO " + kSeratoLibraryTable + " ("
-            + LIBRARYTABLE_TITLE + ", "
-            + LIBRARYTABLE_ARTIST + ", "
-            + LIBRARYTABLE_ALBUM + ", "
-            + LIBRARYTABLE_GENRE + ", "
-            + LIBRARYTABLE_COMMENT + ", "
-            + LIBRARYTABLE_GROUPING + ", "
-            + LIBRARYTABLE_YEAR + ", "
-            + LIBRARYTABLE_DURATION + ", "
-            + LIBRARYTABLE_BITRATE + ", "
-            + LIBRARYTABLE_SAMPLERATE + ", "
-            + LIBRARYTABLE_BPM + ", "
-            + LIBRARYTABLE_KEY + ", "
-            + LIBRARYTABLE_LOCATION + ", "
-            + LIBRARYTABLE_BPM_LOCK + ", "
-            + LIBRARYTABLE_DATETIMEADDED + ", "
+            "INSERT INTO " + kSeratoLibraryTable + " (" + LIBRARYTABLE_TITLE + ", " + LIBRARYTABLE_ARTIST + ", " + LIBRARYTABLE_ALBUM + ", " + LIBRARYTABLE_GENRE + ", " + LIBRARYTABLE_COMMENT + ", " + LIBRARYTABLE_GROUPING + ", " + LIBRARYTABLE_YEAR + ", " + LIBRARYTABLE_DURATION + ", " + LIBRARYTABLE_BITRATE + ", " + LIBRARYTABLE_SAMPLERATE + ", " + LIBRARYTABLE_BPM + ", " + LIBRARYTABLE_KEY + ", " + LIBRARYTABLE_LOCATION + ", " + LIBRARYTABLE_BPM_LOCK + ", " + LIBRARYTABLE_DATETIMEADDED +
+            ", "

Exhibit C

See #2480 (comment)

-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_ARTIST]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_ARTIST);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_TITLE]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_TITLE);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_ALBUM]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_ALBUM);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_ALBUMARTIST]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_ALBUMARTIST);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_YEAR]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_YEAR);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_GENRE]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_GENRE);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_COMPOSER]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COMPOSER);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_GROUPING]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_GROUPING);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_TRACKNUMBER]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_TRACKNUMBER);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_FILETYPE]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_FILETYPE);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_NATIVELOCATION]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_NATIVELOCATION);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_COMMENT]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COMMENT);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_DURATION]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_DURATION);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_BITRATE]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_BITRATE);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_BPM]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_BPM);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_REPLAYGAIN]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_REPLAYGAIN);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_DATETIMEADDED]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_DATETIMEADDED);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_TIMESPLAYED]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_TIMESPLAYED);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_RATING]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_RATING);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_KEY]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_KEY);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_PREVIEW]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_PREVIEW);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_COVERART]
-        = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART);
-    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_POSITION]
-        = fieldIndex(ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_POSITION);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_ARTIST] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_ARTIST);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_TITLE] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_TITLE);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_ALBUM] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_ALBUM);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_ALBUMARTIST] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_ALBUMARTIST);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_YEAR] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_YEAR);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_GENRE] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_GENRE);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_COMPOSER] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COMPOSER);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_GROUPING] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_GROUPING);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_TRACKNUMBER] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_TRACKNUMBER);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_FILETYPE] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_FILETYPE);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_NATIVELOCATION] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_NATIVELOCATION);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_COMMENT] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COMMENT);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_DURATION] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_DURATION);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_BITRATE] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_BITRATE);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_BPM] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_BPM);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_REPLAYGAIN] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_REPLAYGAIN);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_DATETIMEADDED] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_DATETIMEADDED);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_TIMESPLAYED] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_TIMESPLAYED);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_RATING] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_RATING);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_KEY] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_KEY);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_PREVIEW] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_PREVIEW);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_COVERART] = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_COVERART);
+    m_columnIndexBySortColumnId[TrackModel::SortColumnId::SORTCOLUMN_POSITION] = fieldIndex(ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_POSITION);

daschuer · 2020-02-05T20:35:02Z

The trick is

to put the operator at the end of the line
to make a column of function parameters
During testing I have created this apply changes from git-clang-format Holzhaus/mixxx#2

daschuer · 2020-02-05T20:35:29Z

I think we need to update the style guide.

daschuer · 2020-02-05T21:02:38Z

here it is: https://www.mixxx.org/wiki/doku.php/coding_guidelines#line_widths

Holzhaus · 2020-02-05T21:10:48Z

Thanks.

From https://www.mixxx.org/wiki/doku.php/coding_guidelines#line_widths :

Use double indent (8-spaces) for broken lines or align with the opening “(” in the line above for hanging indents.

I guess hanging indents are not possible with clang-format?

uklotzde · 2020-02-05T21:47:24Z

Thanks.

From https://www.mixxx.org/wiki/doku.php/coding_guidelines#line_widths :

Use double indent (8-spaces) for broken lines or align with the opening “(” in the line above for hanging indents.

I guess hanging indents are not possible with clang-format?

No. I deleted the redundant and incorrect sentence.

daschuer · 2020-02-09T00:07:41Z

So we can close this?

Holzhaus · 2020-02-09T01:56:42Z

Yup. If more issues arise we need to adapt the wiki and make sure it doesn't conflict with clang-format.

.clang-format: Set column limit to 80

391954a

Right now, running clang-format or git clang-format will unwrap lines and make them longer than 80 chars, even if the developer inserted linebreaks manually.

Holzhaus added the code quality label Feb 4, 2020

Holzhaus closed this Feb 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.clang-format: Set column limit to 80 #2486

.clang-format: Set column limit to 80 #2486

Holzhaus commented Feb 4, 2020

uklotzde commented Feb 4, 2020 •

edited

Loading

daschuer commented Feb 5, 2020

Holzhaus commented Feb 5, 2020

daschuer commented Feb 5, 2020

Holzhaus commented Feb 5, 2020 •

edited

Loading

daschuer commented Feb 5, 2020

daschuer commented Feb 5, 2020

daschuer commented Feb 5, 2020

Holzhaus commented Feb 5, 2020

uklotzde commented Feb 5, 2020

daschuer commented Feb 9, 2020

Holzhaus commented Feb 9, 2020

.clang-format: Set column limit to 80 #2486

.clang-format: Set column limit to 80 #2486

Conversation

Holzhaus commented Feb 4, 2020

uklotzde commented Feb 4, 2020 • edited Loading

daschuer commented Feb 5, 2020

Holzhaus commented Feb 5, 2020

daschuer commented Feb 5, 2020

Holzhaus commented Feb 5, 2020 • edited Loading

Exhibit A

Exhibit B

Exhibit C

daschuer commented Feb 5, 2020

daschuer commented Feb 5, 2020

daschuer commented Feb 5, 2020

Holzhaus commented Feb 5, 2020

uklotzde commented Feb 5, 2020

daschuer commented Feb 9, 2020

Holzhaus commented Feb 9, 2020

uklotzde commented Feb 4, 2020 •

edited

Loading

Holzhaus commented Feb 5, 2020 •

edited

Loading