-
Notifications
You must be signed in to change notification settings - Fork 126
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
Modernize Humminbird internals #1322
Conversation
robertlipe
commented
Aug 14, 2024
•
edited
Loading
edited
- Modernize Humminbirds internals
This reverts commit cfcf7b3. Apparently .cmake is already there...and the merge of my oneline change made dogmeat out of the file anyway that didn't show with git diff. Man, I hate git.
This survives a $ XMallocStackLogging=true MallocScribble=true XMallocPreScribble=true XMallocGuardEdges=true ./testo on MacOS. ./testo humminbird crash before this.
humminbird.cc
Outdated
wpt->shortname = s; | ||
xfree(s); | ||
|
||
wpt->shortname = QStringFromRaw(sizeof(w.name), w.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this all we need here? If so can we eliminate QStringFromRaw or replace its contents along these lines?
wpt->shortname = QByteArray(w.name, qstrnlen(w.name, sizeof(w.name)));
You could use QByteArray::fromRawData instead but I suggest saving that copy is noise and prone to misuse (source must continue to exist, result not '\0' terminated).
I gave up on this last night. It was driving me crazy as you can tell from
my hit-and-miss attempts to debug it in CI.
I first missed from char* interfaces, but later moved to those.
strnlen might be the ticket. My goal was to get rid of xstrndup as this is
about the only caller left. My final version last night went back to
using xstrndup
and I was still having luck.
I need to go back and be sure that I'm using the struct sizes I should be
using and such. I probaby got carried away with a yank and put and have
they wrong field size or something.
I'll pick it back up tonight after dr. appts. Of course, if that function
goes back to being a one-liner (and I'd forgotten about QByteArray having
"better" ctors here) then this function goes away. I don't know why I
swapped the arguments. Maybe I shouldn't have been programming at all last
night. I was able to get some failures with the debug malloc on MacOS, but
it consistently worked on MacOS and not any other system that we care
about, so it became quite the frustrating hunt.
And, yes, while it's my nature to count clock cycles from so many years of
doing systems software, I'll readily conced that humminbird's speed is
veery low on my list of reasons to not be sleeping.
I'll pick it back up. Thanx for the hint and the pep-talk that this
shouldn't be NEARLY as time-consuming as it was.
…On Wed, Aug 14, 2024 at 8:27 AM tsteven4 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In humminbird.cc
<#1322 (comment)>:
> @@ -247,24 +284,20 @@ HumminbirdBase::humminbird_read_wpt(gbfile* fin)
auto* wpt = new Waypoint;
- // Could probably find a way to eliminate the alloc/copy.
- char* s = xstrndup(w.name, sizeof(w.name));
- wpt->shortname = s;
- xfree(s);
-
+ wpt->shortname = QStringFromRaw(sizeof(w.name), w.name);
Isn't this all we need here? If so can we eliminate QStringFromRaw or
replace its contents along these lines?
wpt->shortname = QByteArray(w.name, qstrnlen(w.name, sizeof(w.name)));
You could use QByteArray::fromRawData instead but I suggest saving that
copy is noise and prone to misuse (source must continue to exist, result
not '\0' terminated).
—
Reply to this email directly, view it on GitHub
<#1322 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC3VAD2B56ETS5UIB4RFDJLZRNLLTAVCNFSM6AAAAABMPL5BH6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMZYGE3DOOBRHA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |