Closed tsteven4 closed 4 weeks ago
tsteven4
commented
4 weeks ago It is disappointing that tidy didn't flag anything about the uninitialized data.
Another way to deal with this is to make sure all classes and structs initialize themselves. This can be done with default member initializers for every POD data member.
GPSBabelDeveloper
commented
4 weeks ago LGTM
I agree with pretty much every point.
Having initializer is clearly "better", but it touched more code than just keeping the calloc style initialization. Since I wasn't even certain that was the problem (I spent hours chasing the wrong problem) so I took a the zalloc approach. I'm not totally in love with that. Having a few dozen sw $r0, 20($t1) in the actors instead. Of a member isn't something I care about in this case. So I went with the low risk approach (missing one) when I was experimenting.
It did indeed such that valgrind or flow analysis warnings didn't find this. It was an expensive problem to fix (that j created because I wanted to kill xstrndup)
I did the edit in clion which isn't my norm. I really hate tyoung the c++ casts so when it warned about them and offered a two click way to fix them, I took it. Things like losing the ability to allocate more the 4 billion hummingbird track points didn't exactly stress me, so I double cast a few places just to hush it.
There are a lot of those gsls that sound good in a new code base but are just a bit eye rolling in an established base like. Ours that began life outside of c++.
So I don't have a strong emotional attachment to much in this chance. I just wanted xstrndup to die as I have a big reduction in code in utile also in the oven.
I think the most horrifying thing I found was the ptr2int stuff that is ultimately used in that format to store ... An int.in extra data.
That format really is our current mapsource that's just full of hurt.
On Thu, Aug 15, 2024, 10:15 AM tsteven4 @.***> wrote:
It is disappointing that tidy didn't flag anything about the uninitialized data.
Another way to deal with this is to make sure all classes and structs initialize themselves. This can be done with default member initializers for every POD data member.
— Reply to this email directly, view it on GitHub https://github.com/GPSBabel/gpsbabel/pull/1324#issuecomment-2291500068, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC3VAD35ZCQMVPPSRMMILJLZRTAYFAVCNFSM6AAAAABMSLWFBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJRGUYDAMBWHA . You are receiving this because you are subscribed to this thread.Message ID: @.***>
The missing initialization was introduced in 824e01d.
I find the introduced static_casts from #1322 to be questionable (not the conversions from c-sytle casts). Are these all narrowing conversions we are trying to cover up? There are still 6 clang tidy related bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions. OTOH I haven't been in a hurry to adopt the gsl https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-narrowing Leaving the implicit narrowing conversions enables tidy to find them.