Tightening the Clang-tidy and Clang-format checks

I need more eye balls to verify the automated bulk update results on my current active dev branch.

This is only the initial bulk update. More will come until everything snaps into place; or not if I failed, and these two jobs that use clang-tidy and clang-format will be removed.

This initial bulk update was from clang-format command only. Let me know if you see anything that is not acceptable for the Urho3D project. Just to be clear, I am not asking for your personal setting.

camera->SetZoom(1.2f * Min((float)graphics->GetWidth() / 1280.0f,
                               (float)graphics->GetHeight() /
                                   800.0f)); // Set zoom according to user's resolution to ensure full visibility
                                             // (initial zoom (1.2) is set for full visibility at 1280x800 resolution)

When the long parameters line is wrapped then it will “align” the next line. There is also option to “not align”. I am OK with both. Currently it is “align” I think. Is that what you objecting?

For future, please also provide the name of the file for easy lookup.

DynamicGeometry::CreateScene()

corrupted table float vertexData[]

void L10n::HandleChangeLanguage(StringHash eventType, VariantMap& eventData)

windowTitle->SetText(l10n->Get("title") + " (" + String(l10n->GetLanguageIndex()) + " " + l10n->GetLanguage() +
                         ")");

Source/Samples/40_Localization/L10n.cpp

This is the same issue. It will wrap when the column limit 120 is hit. The number is configurable. Can set to 0 for don’t wrap. I think it was 80 originally and I changed it to 120. But this wrapping can happen on any number we choose. Unless, if we set it to 0.

Source/Samples/47_Typography/Typography.cpp

corrupted tables const char* levels[] thresholds[] limits[]

void Urho2DStretchableSprite::ScaleSprites(float timeStep)

        const auto scale = Vector2{1.0f + (right  ? quantum
                                           : left ? -quantum
                                                  : 0.0f),
                                   1.0f + (up     ? quantum
                                           : down ? -quantum
                                                  : 0.0f)};

Source/Urho3D/LibraryInfo.cpp

wrong indents in GetCompilerDefines()

what wrong about this wrapping? It is kind of nice If you want may be I could retry without alignment too.

Everywhere, line fragments start with the same indentation, but here it is different

So far the only thing that looks weird and foreign for me is non-multiple-of-4 indentation.
As far as I remember, Urho have never (or almost never) used this kind of formatting in its codebase.

image841×84 4.13 KB

In this particular example it looks good, but in general it tends to waste horizontal line space.
It’s also more annoying to format manually, which may or may not be an issue depending on the editor used by specific person.

Offtopic: I’m forced to use this style of formatting when I’m working with Python code. I don’t know if I like it or not.

TL;DR: I think that multiple-of-4-indentation is more in line with current Urho formatting, and I also personally prefer it due to reasons stated above.

Yes, that the same issue 1vanK is highlighting. It wraps with alignment taking consideration where the previous parameter starts.

OK. Let stop here first. Let me see if I could reconfigure the wrapping setting.

About the column limit, 120 is OK with you guys?

I would do with no limit at all

I am contemplating with that option too. I have LG ultra wide screen monitor, so personally I don’t have any issue with that. Anyway, I don’t think we have too many places with extra long lines, so it should be fine.

++ for 120 column limit, we use the same setting on my dayjob. It really helps at diffs review. Although if you really want to keep it as is… well, no limit worked fine for 15 years.
++ for constructor initializer list refactoring
++ for uniform spacing all over the code

Unsure about alignment, I would prefer to keep 1-tab alignment.
I didn’t read whole commit, so maybe I didn’t notice other changes.

Yes, I agree the column limit is good for diffs review, and also for browsing the code through GitHub web interface.

The limit of 120 is large. Lines that exceed this limit do so for some reason. Do you really want to interfere with this? If you really want to set a high limit, then don’t set a limit at all.

120 symbols is not “huge” limit, it is exactly how much fits on Full HD monitor in Beyond Compare for 2-way diff. 120 symbols is also how much code fits on average laptop display with IDE side bar.

120 or 130 symbols is often used as middle ground between unlimited lines and 80-char limit from DOS epoch

80 is too little, especially if you want to apply it to legacy codebase that was written without this limit in mind.

No limit works okay as long as people writing code stay reasonable and keep their lines generally short.

okay, another question, now we have a lot of generated code, should I now complicate the generator, calculate the length of each line (currently I just write to stream directly), etc. or will clan-tidy constantly struggle with the generator?

I’m 99% certain that generated code should be extempt from auto formatting in any form as it is not supposed to be read or edited by a person.

I just did a bit of math. 97% of Source/Urho3D/ is already under 120 char limit. 98% is under 130.
It’s gonna be about 4-6k lines of code changed due to enforcing line limit. It’s a lot, but it’s probably okay.

Generated code is human readable. But in fact I should integrate own realization clang-format into generator to full match.

I suggest to disable formatting for arrays altogether

Source/Urho3D/Graphics/Material.cpp

textureUnitNames[] - array with define

Thanks for all your inputs. I will try to fine-tune the setting later. I may or may not be able to find the right switch for each issue. So, just like the clang-tidy, we may not always get exactly what we want. The goal is to find the right mix that could get the acceptable overall result.

For the generated code, Lua bindings are excluded already simply because they are generated on the fly and not in the source tree. The new AS bindings are included though at the moment. However, they could be easily excluded too.

I also plan to have separate workflow that would be triggered by scheduler, something like a nightly batch. In this workflow, I could have one job to do the same bulk update that I was doing manually earlier. This would allow maintainers to still merge a PR even though there are still small formatting errors. Instead of forcing the PR author to rectify, we can let the nightly batch to take care of them automatically. This is all provided we can agree on the acceptable settings.

Clang-format moves content of namespaces to first column. Sometime namespace used as “singleton” and moving is not required:
Source/Tools/BindingGenerator/XmlSourceData.cpp -> namespace SourceData

Source/Urho3D/Graphics/ShaderVariation.cpp

ShaderParameter::ShaderParameter(const String& name, unsigned glType, int location)
    : // NOLINT(hicpp-member-init)
    name_{name}
    , glType_{glType}
    , location_{location}
{

}

The string continuation looks correctly indented to me.

The namespace indentation is set to none. This should give similar result as Urho3D namespace. The other options is “inner” and “all”. The “all” may give you the indent even on the first level, but that would change the code base all over the place including the Urho3D namespace.

Maybe don’t use namespaces as singletons then?

If user is okay with non-indented content, they can use namespace.
If user is not okay with it, they can use struct or class.

Indenting all the namespaces is not an option, it would affect 99% of Urho codebase and waste a lot of line space.

Disabling namespace formatting is an option, but I don’t think it’s good idea to surrender automatic formatter for sake of 10 lines somewhere in code.

I am withdrawing from this discussion, I don’t like this auto-formatting idea at all

If a smart auto-formatting system decides for me how my code should look like, where auto should be, and so on, then let the system write this code

It’s offtopic, but have you ever tried writing in Python?
They have this whole PEP thing, which is basically clang-format, except it’s turned on by default in all modern IDEs and much more strict.
Whole language community is de-facto required to write the code in single style.
Sometimes I wish C++ had something like that.

I like auto formatting as long as it is conforming with the existing codebase (doesn’t cause too much changes), but I can live without it.

In that case then I will just stop. Do what you like with the two jobs. I don’t care anymore. I hereby declare I have completed the migration work even though it is not yet finished.

Yes, I have modified the exporter for Blender 2.8. Python is a disgusting language.

There is one useful thing - clearing lines that only have spaces.

I don’t think I can continue if only formatting already caused so much contention. And, I have actually just started and haven’t done anything with its big cousin, clang-tidy, yet. So, let’s can this first. I have other better things to do too.

For what its worth i think auto-formatting is a good idea. It may not be simple to achieve. It may not be perfect in all edgecases. But if it can be done, it would save a lot of time for contributors and maintainers alike. Personal likes or dislikes of minority should not be stopping progress of a project.

@1vanK that is some rather toxic attitude. Instead of trying to find a middle ground you chose to (╯°□°）╯︵ ┻━┻ instead. This is not something people expect from a moderator.

In my view, it means less attention spent on code, and more on bla. It’s like having a garbage collector; why are you making a mess?

Also, table flips represent break-throughs, but you’d have to be a dreamer to understand that.

The auto formatting can be disabled locally to keep the the original author intent if it is absolutely needed to keep the format verbatim. Like I said, I just barely started. But until we have a true team lead that everyone blindly obey in this matter, I see this kind of discussion is pointless as everyone has personal own taste. So I am closing this thread.

Instead of trying to find a middle ground you chose to create a separate fork and split the community.

Respect the compulsive proctologist.