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

[RDY] Cpp fmt #1562

Merged
merged 13 commits into from Oct 6, 2019

Conversation

@TheCycoONE
Copy link
Member

commented Sep 11, 2019

Introduces clang-format and new style for C++, based closely on Googles style.

Rules not enforced by clang-format e.g. including braces on conditionals are not yet included.

Some other minor issues (compiler warnings, non-portable casts) are resolved in this patch set.

TheCycoONE added 9 commits Jun 12, 2019
* Break up / shorten some long lines

* Add some missing braces for clarity

* Multiline strings are merged to let clang-format split them
appropriately.

* sdl_core's frame_count changed from a C style array to std::array
which made the length checks simpler.

* Add includes and forward declairs to avoid transitive dependencies

* Remove th_gfx_font.h include from th_gfx.h - circular dependencies

* using to shorten lines in th_map.cpp

* Avoid non-portable reinterpret_cast for parcels in th_map.

* Use more constants in th_map.

* Use class initializer for th_map classes
Also includes some manual braces.
Anonymous structs in anonymous unions are not supported by the standard.
@TheCycoONE TheCycoONE force-pushed the TheCycoONE:cpp-fmt branch from 41b1965 to 4c5bed0 Sep 12, 2019
@TheCycoONE TheCycoONE changed the title Cpp fmt [RDY] Cpp fmt Sep 12, 2019
pFiles->Add(pTmp = new wxButton(this, ID_NEXT, L"Next"), 0, wxALIGN_CENTER | wxALL, 1);
wxButton* pTmp;
pFiles->Add(
pTmp = new wxButton(this, ID_LOAD, L"Load Simple"),

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Sep 22, 2019

Contributor

Code is being really silly here, at least I don't see the need for pTmp.

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Sep 22, 2019

Author Member

Out of scope - I got in a trap of refactoring everything earlier, but it can always be done later.

m_txtData->GetValue().IsEmpty()
? m_txtTable->GetValue().BeforeLast('.') + L".DAT"
: m_txtData->GetValue()) ||
!m_oAnims.loadPaletteFile(m_txtPalette->GetValue())) {

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Sep 22, 2019

Contributor

Indentation of this line versus the next one is wrong imho. There should always be a distinction between code in the condition and code in the 'then' part.

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Sep 22, 2019

Author Member

This example has a condition that's far too long to be sane, it should be split up and put in it's own function.

I agree, a style that would require or allow a blank line at the top of the code in the condition in this case would be better. (Google style does allow it, but clang-format doesn't.) In the end I opted to stop fighting the formatter.

Rather than fight it, shorten the conditional so it's not confusing.

0x03C0, 0x03A3, 0x03C3, 0x03BC, 0x03C4, 0x03A6, 0x0398, 0x03A9, 0x03B4,
0x221E, 0x03C6, 0x03B5, 0x2229, 0x2261, 0x00B1, 0x2265, 0x2264, 0x2320,
0x2321, 0x00F7, 0x2248, 0x00B0, 0x2219, 0x00B7, 0x221A, 0x207F, 0x00B2,
0x25A0, 0x00A0};

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Sep 22, 2019

Contributor

Pretty sure 8 values at a line makes much more sense for 0x80 numbers.

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Sep 22, 2019

Author Member

Yes, I have an override for this case elsewhere, should add it here as well.

{
if(!read_byte_stream(reinterpret_cast<uint8_t*>(&fValue), sizeof(T)))
bool read_float(T& fValue) {
if (!read_byte_stream(reinterpret_cast<uint8_t*>(&fValue), sizeof(T)))

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Sep 22, 2019

Contributor

Code can be shorter by returning the value of the function call, but for formatting, does it make sense to have multi-line statements without curly brackets?

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Sep 22, 2019

Author Member

No, the new style requires braces here; just wasn't automatically picked up and I missed it.

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Sep 22, 2019

Author Member

It's only a single line - Google style allows it.

mt[i] = ((uint32_t*)data)[i];
const char* data = lua_tolstring(L, 1, &len);
if (len != N * 4 + 2) luaL_argerror(L, 1, "Seed string wrong length");
for (i = 0; i < N; ++i) mt[i] = ((uint32_t*)data)[i];

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Sep 22, 2019

Contributor

Many function calls are split in a lot of lines, yet this is allowed. While I like this compact style, not sure if it fits in the general idea of the used style.

&& are_ranges_equal(0, iNumRepeats, iOffset, iObjSize))
{
while (iObjSize * (iOffset + iNumRepeats + 1) <=
buffer_size &&

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Sep 22, 2019

Contributor

Really weird breaking point.

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Sep 22, 2019

Author Member

Doesn't look weird to me if buffer_size didn't fit on the line, it's the least connected to what's before it, and the expressions left and right of the && are even less connected.

That said, again, a well named function (or functor, or lamba) for the conditional would help readability.

@@ -28,15 +28,18 @@ SOFTWARE.
#include FT_FREETYPE_H
#endif

class render_target;

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Sep 22, 2019

Contributor

empty line doesn't look useful to me

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Sep 22, 2019

Author Member

It's consistent with a rule about leaving a blank line of white space between class definitions. Perhaps not useful in this case, but not worth an override either imho.

breaks like `\r` and `\n` in sMessage are ignored), but inserts line breaks
between words so that no single line is wider than iWidth pixels.
breaks like `\r` and `\n` in sMessage are ignored), but inserts line
breaks between words so that no single line is wider than iWidth pixels.

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Sep 22, 2019

Contributor

Why is the indenting different?

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Sep 22, 2019

Author Member

There's a few of these: seems that clang-format doesn't understand this style of comment.

@Alberth289346

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

I don't quite like the large amount of lines that get created with function definitions and function applications, it's systematic and all that, but having two lines for int iX, int iY is more clutter than useful imho.

For function applications, I can see the idea, especially if an argument is more than a simple variable or value. For function definitions, I see no use. There is a piece of documentation that explains each parameter in calling order already.

@TheCycoONE TheCycoONE changed the title [RDY] Cpp fmt [WIP] Cpp fmt Sep 22, 2019
@TheCycoONE

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2019

Fine, that's bin packing, and it was an override I did make based on personal taste - but I'll disable it and learn to live with it.

TheCycoONE added 4 commits Sep 22, 2019
2 space indent, no forced break on braces.
Order of usings in config.h.in since 8 is smaller than 16.
Table layout, since 0x80 nicely fits in 8 columns.
@TheCycoONE

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2019

Went full Google, because I want to avoid nit picking more than I care what style is used; and clang-format makes that an easy option.

@TheCycoONE TheCycoONE changed the title [WIP] Cpp fmt [RDY] Cpp fmt Sep 22, 2019
@Alberth289346

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2019

2-space indenting??

Ah well, I give up, do what you think is good and just merge.

@TheCycoONE TheCycoONE merged commit 4a1c98a into CorsixTH:master Oct 6, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.