Humminbirdhash

[tsteven4]

• refactor humminbird handling of wpt numbers. This fixed some subtle bugs.

  1. The map was shared and not cleared allowing communication between invocations of the humminbird reader & writer (reader->reader, reader->writer, writer->writer, etc.)
  2. When reading, waypoints that were dropped due to their status were still registered in the map. If they happened to referred to by a route then deallocated memory would be accessed.
  3. humminbird_write_waypt_wrapper checked the map using the [] operator, which causes a silent insertion. However, in this case the silently inserted entry was overwritten a few lines later with the correct value.

  It also allows gb_int2ptr and gb_ptr2int to be retired.

• convert some macros to constexpr.

• consistently use strncpy to move strings to humminbird char arrays.`

• fix clang-diagnostic-missing-field-initializers

[GPSBabelDeveloper]

Interesting. I was using CLion and the version that shipped last week, after I did this work, has default clang warnings WAY too hyper for me. I've been aggressively hushing many of the silly ones.

No, I don't think I'll be accepting trailing return types on every method we define. Just No.

So I got lucky and "fixed" the failure in CI by setting only the first member, num, to zero instead of everything to zero. I remember seeing discussion on the nuances between aggregate initialization and direct initialization but now that I look carefully at examples like:

std::vector vec0(5, 9); // 9, 9, 9, 9, 9 std::vector vec1{5, 9}; // 5, 9 I just want to cry. Gross! If you don't make those structs into "real" classes with initializers, I will. This is a bomb waiting for us again.

...and I don't even LIKE humminbird. It was already hanging on by a thread.

On Mon, Aug 19, 2024 at 2:40 PM tsteven4 @.***> wrote:

@.**** commented on this pull request.

In humminbird.cc https://github.com/GPSBabel/gpsbabel/pull/1327#discussion_r1722264275:

@@ -229,7 +223,7 @@ HumminbirdBase::humminbird_rd_deinit() const void HumminbirdBase::humminbird_read_wpt(gbfile* fin) {

• humminbird_waypt_t w{0};
• humminbird_waypt_t w{};

This was a resharper suggested fix I agree with, although the previous code worked fine. It does DTRT.

In the case you cite I believe the new code uses value initialization https://en.cppreference.com/w/cpp/language/value_initialization. Although in this case as the note says aggregate initialization https://en.cppreference.com/w/cpp/language/aggregate_initialization is performed. Thus I think the difference is {0} has an explicit initializer for num, and the rest of the fields are implicitly initialized, while with {} all fields are implicitly initialized.

The warning "clang-diagnostic-missing-field-initializers" was telling us there were fields that weren't explicitly initialized and at least one field that was explicitly initialized. The warning does not apply to an empty initializer.

There certainly were test issues, and similar issues that didn't have test coverage, related to uninitialized data. All the xcalloc (not xmalloc) calls are a hint that someone in the past thought those needed to be initialized. You applied the fix in some additional cases, but that isn't a bad thing Always initialize an object https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es20-always-initialize-an-object. It is too easy to miss a field in something like humminbird_read_wpt. The memset/memcpy to strncpy change is relying on that for name[19].

— Reply to this email directly, view it on GitHub https://github.com/GPSBabel/gpsbabel/pull/1327#discussion_r1722264275, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC3VAD7FWRW2KY7JL4UTB7LZSJCZNAVCNFSM6AAAAABMYATMDWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENBWGMZDCOJSHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[tsteven4]

So I got lucky and "fixed" the failure in CI by setting only the first member, num, to zero instead of everything to zero.

No, you had initialized all members. You provided an explicit initializer for the first member, but the rest used implicit initializers.

Implicitly initialized elements For a non-union aggregate, each element that is not an explicitly initialized element is initialized as follows ...

As for the keywords struct vs. class they are nearly identical (https://en.cppreference.com/w/cpp/language/class)

The keywords class and struct are identical except for the default member access and the default base class access

The cppcoreguidelines suggest using struct or class based on:

Use class if the class has an invariant; use struct if the data members can vary independently

but generally we don't follow this suggestion.

If you don't make those structs into "real" classes with initializers, I will. This is a bomb waiting for us again.

I don't see a need to declare these using class instead of struct. However, providing default brace initializers for all non-class members is something to consider. It is more verbose in the struct/class definition, but every usage is initialized. I generally don't provide a default initializer for fields that are classes, rely on the class itself to provide an appropriate default, but provide default member initializers for non-class fields, e.g. "int x{};" or if I rely on a particular initial value "int y{22.33};" For an example search for "Data Members" in kml.h.

[tsteven4]

There has been recent interest in humminbird, so I don't think we should axe it.