embeddedartistry / libc

libc targeted for embedded systems usage. Reduced set of functionality (due to embedded nature). Chosen for portability and quick bringup.
MIT License
510 stars 67 forks source link

Satisfying clang-tidy has led to insane code choices #enhancement #118

Closed johnwbyrd closed 4 years ago

johnwbyrd commented 4 years ago

Sometime around 2018, you decided to make a ton of changes to the code base of the following form, for example in https://github.com/embeddedartistry/libc/blob/master/src/string/strcspn.c :

    for(; *c && BITOP(byteset, *(const unsigned char*)c, |=); c++)
    {
        {
            ;
        }
    }

You did this in order to satisfy some warning message or other from clang-tidy.

Literally no programmer worth their salt would consider the above code to be superior to the obvious implementation:

    for(; *c && BITOP(byteset, *(const unsigned char*)c, |=); c++)
                ;

Look: clang-tidy's complaints should NEVER be a substitute for writing clear and understandable code, ESPECIALLY in a libc. If clang-tidy is providing insane output, then you need to adjust your clang-tidy flags, not change the entire code base to conform to a convention that makes no sense.

I was getting ready to use this libc in a project until I saw you making this sort of change. But these types of modifications make me doubt the sanity and dependability of this library.

Clear, simple, fast, understandable code. THAT must be the goal of any embedded libc. Not satisfying whatever nonsense clang-tidy spits out.

phillipjohnston commented 4 years ago

Thanks for your lecture. As I explained already in the commit, it was a clang-format mistake that I noticed recently and have been correcting.

The fact that you choose to insult my intelligence and abilities as a result of this is unacceptable.

johnwbyrd commented 4 years ago

At least it's something that you recognize that this sort of change is not a good one. I insulted the code, not you.

phillipjohnston commented 4 years ago

Programmer, not code:

Literally no programmer worth their salt would consider the above code to be superior to the obvious implementation

You, not code:

Look: clang-tidy's complaints should NEVER be a substitute for writing clear and understandable code, ESPECIALLY in a libc. If clang-tidy is providing insane output, then you need to adjust your clang-tidy flags, not change the entire code base to conform to a convention that makes no sense.

phillipjohnston commented 4 years ago

Hey man, you opened the issue, and I'm going to fix it all right now.

phillipjohnston commented 4 years ago

FWIW, I recommend Keith Packard's picolibc as an alternate to this one: https://github.com/keith-packard/picolibc.

Now, for clarification, it is the style of this project to not use single-line conditionals without braces. There are problems with the "obvious" implementation, here is one example explaining them. Many coding styles follow this recommendation.

Adding doubled sets of braces, as explained in the commit you commented on before opening this issue, was a clang-format error. The remaining instances of the error have been corrected.

That this is the reason that you chose to be so insulting and derogatory, and to say that you wouldn't use this libc, is laughable. Was it an error? Absolutely. But there was absolutely no performance or behavioral impact to this auto-formatting change.

I hope that your code is as perfect and free of errors as your attitude suggests it is.