Numerous minor changes for correctness. #648
Conversation
I know this project isn't fond of these, but the screeching of loading this project into Clion or Neovim or any other modern tooling is too much. PRs like this are eating veggies. They may not be tasty like features, but they're good for long-term health. I won't break line-by-line here, so I'll just get big picture. globals.h was basically including the world when it doesn't need to. I reduced the amount of tokens exposed to each compilation unit by simplifying globals.h and pushing more of an include-what-you-use style. Not everything is totally hermetic, but some files that I preprocessed are now seeing 20-40,000 lines fewer of headers each. I tried to preserve the CP/M line endings in files that are blessed with those. (Dare me to fix that in a future PR, please....) For both English and C++, almost every change was from "you know what I mean" to "what the reader actually read it as". * Some typos are fixed. * Some "bool foo = 1" became "boo foo = true". * Ctors with a single argument became explicit. (Accidentally pass an integer that gets promoted to a pointer and then used as such for a good time.) * Operator = was changed to handle the self setting case when needed. * The second argument to strtoul and friends is a pointer, not an integer. nullptr > NULL for these. * Default ctors and dtors, overrides, etc. are now spelled post C++11 ways. * When I was pretty sure that code was really dead, I removed it. LOTS remain. * Prefer foo.empty() over foo.size == 0. Think testing the zflag instead of a cmp #0. The container might have to work hard to know the actual size, but know quickly that it has something. * When header blocks were ugly enough to ooze, I sorted them. The rest probably need that on some rainy day. * Sometimes when implicit container iteration spoke to me, I listened. May we never again have to figure out of a matrix dimesion is signed or unsigned. (LOL - we still do....) * Consts and statics are labeled more aggressively. * When loops were making read-only temp copies, I worked to look at the callers refs instead. * Things that were scary (like improv) I touched only very lightly. I don't care about remotecontrol because I have a replacement pending. screen.cpp doesn't bother me because I've not ported to my S3-BOX. Fields that seem unused in places that may appear in a packet stayed. Since I don't edit those files, the warnings in them don't bother me. :-) * When I disagree with the compiler, it loses. I LIKE 'if (3 == foo)'. and [[nodiscard]] is dumb. I'm not a total sellout. * I didn't tackle big picture mismatches like "is time a double or a long". Those just spill over and downcast all over. Probably way more, but most everything SHOULD be reviewable by just paging through, nodding your head, tagging a few things to come back to in the future if not in this PR (build times are just awful...) but everything should be pretty mechanical and self-evident. If you want to take it "except for ...", please feel free to just edit the PR. I can probably explain anything individually, but I'm probably not so emotionaolly attached to saving one copy on a loop that I'll let the whole thing die - or force either of us to spend the time for multiple builds. Those are getting crushing. This whole PR was cherry-picked from my trees that have been running mesmerizer, demo, a superset of hex, and a superset of radial circle, and spectrum. They all seem fine, but I can't promise to have inspected every pixel on every build. (I'm skeptical anyone can.) Successful build: ===================== [SUCCESS] Took 117.20 seconds ===================== Environment Status Duration ------------------------ -------- ------------ demo SUCCESS 00:00:12.596 m5demo SUCCESS 00:00:45.958 m5plusdemo SUCCESS 00:00:45.343 m5stackdemo SUCCESS 00:00:49.577 heltecdemo SUCCESS 00:01:08.584 heltecv2demo SUCCESS 00:00:46.029 heltecv3demo SUCCESS 00:00:48.808 lilygo-tdisplay-s3-demo SUCCESS 00:00:32.880 ledstrip_amoled SUCCESS 00:00:49.330 ledstrip SUCCESS 00:00:49.323 ledstrip_feather SUCCESS 00:00:34.618 ledstrip_feather_hexagon SUCCESS 00:00:34.183 ledstrip_feather_wrover SUCCESS 00:00:31.670 ledstriplite SUCCESS 00:00:33.466 ledstrip_pico SUCCESS 00:00:32.423 spectrum SUCCESS 00:00:42.362 spectrum2 SUCCESS 00:00:42.649 helmet SUCCESS 00:00:42.585 spectrum_elecrow SUCCESS 00:00:56.125 spectrumstack SUCCESS 00:00:42.633 spectrumlite SUCCESS 00:00:45.230 spectrum_wrover_kit SUCCESS 00:00:34.408 mesmerizer SUCCESS 00:00:35.640 mesmerizer_lolin SUCCESS 00:00:54.078 laserline SUCCESS 00:00:37.684 lantern SUCCESS 00:00:41.808 pdpgrid SUCCESS 00:01:57.374 chieftain SUCCESS 00:01:31.571 umbrella SUCCESS 00:01:28.386 generic SUCCESS 00:01:34.298 panlee SUCCESS 00:01:45.128 ttgo SUCCESS 00:01:30.101 xmastrees SUCCESS 00:01:47.136 insulators SUCCESS 00:01:39.503 magicmirror SUCCESS 00:01:46.613 atomlight SUCCESS 00:01:26.652 spirallamp SUCCESS 00:01:46.936 fanset SUCCESS 00:01:58.392 cube SUCCESS 00:01:57.196 ====================== 39 succeeded in 00:40:19.275 ======================
|
Hi @robertlipe, I was wondering how/to what extent you've tested these changes? |
|
TL;DR "pretty well".
These have all been in my work tree for a while; this is just a mechanical
cherry-pick from my trees, withholding the parts that require "real"
thought. Some of them for months. I've tested Demo (extended out to
1024px), Hex (a superset of what's in the tree), a circlle display pattern
(another pattern I have in the oven that, like Hex, is a disturbing mix of
strip + matrix) , and Mesmerizer on real Mesmerizer hardware. For feature
packs, I've touch-tested IR, the telnet daemon, the web interface, etc.,
and had each running cumulatively at least over"night" in some combination
or another YULC is probably the hardware with the longest collective
runtime as it's what I've been working on. That's an S3 board
that's somewhat like a Lilygo-Tdisplay-S3 - "display" + other hardware
intentionally not stirred in here. Obviously, I've done the full build
both locally and in the presubmit. I don't have equipment to test things
like tiny LCD screens.
The two holes I know I have that I understand you can hit easily are
Spectrum (I understand you have some Lilys or Espressif Stacks with audio
inputs) and the mode that takes colorserver packets from the wire. (I don't
do C#....I should probably teach WLED enough about colorserver to fill that
hole for us if necessary..) I've not actually attached blinkies to run
every possible configuration, but I'll say in good faith that it's tested
better than most PRs you'll receive in a week.
I won't say dumb things like it's impossible that I broke something. If I
did, I'll fix it.
If I need to test COLORSERVER, I'll write patterns via nc or something. I'm
acutely aware of the absence of great automated tests for us, so I make it
a point to continuously rotate through what I am building. I've been
thinking about that a lot lately and make have some breaking (lol) ideas
there. Distracting idea: use a cheapo logic analyzer (a word combination
that made no sense just a few decades ago) to gobble up the pins and
compare at least somewhat to collected "test patterns". Something like these
<https://www.aliexpress.us/item/3256805680874886.html> fellas + sigrok is
an idea I've been knocking around. Clock recovery is hard in software, but
dedicated hardware MIGHT make that work.
I know it looks like a lot, but with an hour to build the tree for each,
splitting that into 478 smaller patches would have killed us all.
It's obviously an accumulation that I've had going for a while and was a
long running mixture of lovingly hand-crafted tweaks and "apply quick fix"
from Clion/ Sublime/ Neovim, often while I was trying to work out LSP
tooling for cross dev for each of those editors. There were a few times I
picked through the standard (well, the cppreference version) to suss things
out but mostly I trust g++ to tell me when an include isn't actually used,
for example. If it whines about it, it seems like a good idea, I can
delete it, the world still builds, I went with it.
I tried to remove sharp edges even when they're well defined. For example,
a loop iterator named the same as a class member actually has a defined
conflict resolution scheme. Let's just not do that. As counterpoint, some
of the things the tools are whining about actually are so outrageously dumb
that it's going to be worth figuring out how to turn them off in eleventy
different editors and tools.
[[nodiscard]] const auto& getFoo() { return foo_; }
is just dumb. That's worth fighting the new default. There are a couple of
those to work through. Zero of the "battle insanely dumb dfaults" are are
in this PR.
We had special tooling to deal with this class of change at Google, but
I'll admit I that I don't know of great ways to handle these in open-source
land. I intentionally held back most changes in effects/ in this PR exactly
to reduce merge noise. You kind of want to review a recipe (e.g.
bouffalolab/bouffalo_sdk@bc7c73d)
but that doesn't work for edits like this. Tips on making it easier on you
are welcome. I probably have another batch that's about this size that's
slightly more contentious. Actually bartering out "_Gfx is technically a
reserved identifier. Should we change it or should we have our tools whine
about it for future years? (Even Raymond Chen agrees that Microsoft was
slow to get this memo internally, though it's part of the C89 spec...) I
tried to keep this round the simple (almost) no-logic change version that
aren't likely to be disputed without bikeshedding.
I didn't even fix the spelling of Cylon, for example, just because I've not
edited that file and been bugged by the warnings.. :-)
So to the actual question asked, I feel pretty good about these, but I've
been wrong before and will probably be wrong again about
something....eventually. ;-).
If there's something specific you'd like verified that you'd like me to
perform, feel free to ask.
LMK how to help make things like these go down easily so we can be rested
and hydrated for a good-ole 0xf0rd comma debate.
G'night!
…On Sun, Aug 25, 2024 at 6:37 AM Rutger van Bergen ***@***.***> wrote:
Hi @robertlipe <https://github.com/robertlipe>, I was wondering how/to
what extent you've tested these changes?
—
Reply to this email directly, view it on GitHub
<#648 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD3ZMDOU27SUVQUIDDA3ZTG6WPAVCNFSM6AAAAABM37MCO6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBYG44TGNJRGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
For me the main thing is that I hate breaking functionally working code to implement improvements that may not even be contested in themselves, but are "only" improvements on a non-functional level. At a 99.5% chance of over-explaining, the reason is that in my mind the only real reason for code to be there is to do the stuff it does. There are almost always multiple ways to do stuff and some are better than others, but if the code that's there works and the best-case scenario from a functional perspective is that it does exactly what it does now, then I tend to stick to "if it ain't (functionally) broken, don't try to fix it." In a "binary world" that would put me on the track of "thanks, I'll take the comment changes, but let's just leave the code alone until we have another reason to change it". However, I do understand the value of maintainability, consistency, testability (yes, I read ahead), etc. so I'm not going to make that the conclusion just like that. It does mean that I think we should indeed know we've tested all code paths we're touching at least once, and can't really have any known blind spots there. As responsible guardians of our users' functionality, if you will. And I know, most if not all of these changes should "just work", but you and I have both been doing this long enough to have collected a beautiful batch of campfire stories where it should have just worked... and didn't. So, if testing blind spots do exist (like the colorserver example you mention), then let's cover those before we proceed. And colorserver is a great example, because I also don't use that - which is why I don't think I've ever touched its code in my three years on this project. If we can "casually" make the way we cover them automated and repeatable then one could successfully argue with me that we actually have a reason to touch the code. Concerning the contestability of the changes in this PR at a non-functional level: in my view many of them are clearly improvements because the English language dictates that words are spelled in a certain way, because Which kind of brings me to the question about what makes changes like these more digestible for me, in decreasing order of importance: 1., 2. and 3. Make sure you've tested the new version of the old working code and have seen it working Also, non-functional changes that are part of another "objective" improvement are always easier for me to be comfortable with, if only because that tends to cover the first criterion. I don't know if I've covered all points you asked my input on with this; feel free to point out any that I missed. Cheers, |
|
@robertlipe May I suggest we reopen this? I really would like to incorporate these changes to the extent we (that includes me personally) can test them, and I that's actually the majority of them. |
I know this project isn't fond of these, but the screeching of loading this project into Clion or Neovim or any other modern tooling is too much. PRs like this are eating veggies. They may not be tasty like features, but they're good for long-term health. I won't break line-by-line here, so I'll just get big picture. globals.h was basically including the world when it doesn't need to. I reduced the amount of tokens exposed to each compilation unit by simplifying globals.h and pushing more of an include-what-you-use style. Not everything is totally hermetic, but some files that I preprocessed are now seeing 20-40,000 lines fewer of headers each. I tried to preserve the CP/M line endings in files that are blessed with those. (Dare me to fix that in a future PR, please....) For both English and C++, almost every change was from "you know what I mean" to "what the reader actually read it as". * Some typos are fixed. * Some "bool foo = 1" became "boo foo = true". * Ctors with a single argument became explicit. (Accidentally pass an integer that gets promoted to a pointer and then used as such for a good time.) * Operator = was changed to handle the self setting case when needed. * The second argument to strtoul and friends is a pointer, not an integer. nullptr > NULL for these. * Default ctors and dtors, overrides, etc. are now spelled post C++11 ways. * When I was pretty sure that code was really dead, I removed it. LOTS remain. * Prefer foo.empty() over foo.size == 0. Think testing the zflag instead of a cmp #0. The container might have to work hard to know the actual size, but know quickly that it has something. * When header blocks were ugly enough to ooze, I sorted them. The rest probably need that on some rainy day. * Sometimes when implicit container iteration spoke to me, I listened. May we never again have to figure out of a matrix dimesion is signed or unsigned. (LOL - we still do....) * Consts and statics are labeled more aggressively. * When loops were making read-only temp copies, I worked to look at the callers refs instead. * Things that were scary (like improv) I touched only very lightly. I don't care about remotecontrol because I have a replacement pending. screen.cpp doesn't bother me because I've not ported to my S3-BOX. Fields that seem unused in places that may appear in a packet stayed. Since I don't edit those files, the warnings in them don't bother me. :-) * When I disagree with the compiler, it loses. I LIKE 'if (3 == foo)'. and [[nodiscard]] is dumb. I'm not a total sellout. * I didn't tackle big picture mismatches like "is time a double or a long". Those just spill over and downcast all over. Probably way more, but most everything SHOULD be reviewable by just paging through, nodding your head, tagging a few things to come back to in the future if not in this PR (build times are just awful...) but everything should be pretty mechanical and self-evident. If you want to take it "except for ...", please feel free to just edit the PR. I can probably explain anything individually, but I'm probably not so emotionaolly attached to saving one copy on a loop that I'll let the whole thing die - or force either of us to spend the time for multiple builds. Those are getting crushing. This whole PR was cherry-picked from my trees that have been running mesmerizer, demo, a superset of hex, and a superset of radial circle, and spectrum. They all seem fine, but I can't promise to have inspected every pixel on every build. (I'm skeptical anyone can.) Successful build: ===================== [SUCCESS] Took 117.20 seconds ===================== Environment Status Duration ------------------------ -------- ------------ demo SUCCESS 00:00:12.596 m5demo SUCCESS 00:00:45.958 m5plusdemo SUCCESS 00:00:45.343 m5stackdemo SUCCESS 00:00:49.577 heltecdemo SUCCESS 00:01:08.584 heltecv2demo SUCCESS 00:00:46.029 heltecv3demo SUCCESS 00:00:48.808 lilygo-tdisplay-s3-demo SUCCESS 00:00:32.880 ledstrip_amoled SUCCESS 00:00:49.330 ledstrip SUCCESS 00:00:49.323 ledstrip_feather SUCCESS 00:00:34.618 ledstrip_feather_hexagon SUCCESS 00:00:34.183 ledstrip_feather_wrover SUCCESS 00:00:31.670 ledstriplite SUCCESS 00:00:33.466 ledstrip_pico SUCCESS 00:00:32.423 spectrum SUCCESS 00:00:42.362 spectrum2 SUCCESS 00:00:42.649 helmet SUCCESS 00:00:42.585 spectrum_elecrow SUCCESS 00:00:56.125 spectrumstack SUCCESS 00:00:42.633 spectrumlite SUCCESS 00:00:45.230 spectrum_wrover_kit SUCCESS 00:00:34.408 mesmerizer SUCCESS 00:00:35.640 mesmerizer_lolin SUCCESS 00:00:54.078 laserline SUCCESS 00:00:37.684 lantern SUCCESS 00:00:41.808 pdpgrid SUCCESS 00:01:57.374 chieftain SUCCESS 00:01:31.571 umbrella SUCCESS 00:01:28.386 generic SUCCESS 00:01:34.298 panlee SUCCESS 00:01:45.128 ttgo SUCCESS 00:01:30.101 xmastrees SUCCESS 00:01:47.136 insulators SUCCESS 00:01:39.503 magicmirror SUCCESS 00:01:46.613 atomlight SUCCESS 00:01:26.652 spirallamp SUCCESS 00:01:46.936 fanset SUCCESS 00:01:58.392 cube SUCCESS 00:01:57.196 ====================== 39 succeeded in 00:40:19.275 ======================
|
I'm reopening this myself - I noticed the PR was created from a branch in my own fork, so I feel I'm not out of line by doing so. Compared to the original submission by @robertlipe:
All other changes included in the original PR have been tested by myself using the Mesmerizer and Spectrum (for M5StickC Plus) configurations. |
|
As I prepare to merge this, I'm hereby documenting for posterity that I'll restore the originally proposed changes to socketserver.h in this PR's head branch after this PR has been merged. That way they can still be applied after someone has been able to confirm on actual hardware that the changes don't break anything. |
Description
Contributing requirements
mainas the target branch.