search

uber / h3

Hexagonal hierarchical geospatial indexing system
https://h3geo.org
Apache License 2.0
4.84k stars 459 forks source link

Add ALWAYS and NEVER macros (from SQLite) #720

Closed isaacbrodsky closed 1 year ago

isaacbrodsky commented 1 year ago

Adds ALWAYS, NEVER, and related macros from SQLite (as linked in the code comments) to help ensure fuzzers or unit tests that reach expected-unreachable code appropriately assert. This also excludes defensive code blocks from coverage metrics.

coveralls commented 1 year ago

Coverage Status

Coverage decreased (-0.4%) to 98.597% when pulling 98a747af6a08bcd7481d96f565291a9bb8b5acef on isaacbrodsky:always-macro into da5b21fd0a9812100f234be3769d318e20183915 on uber:master.

isaacbrodsky commented 1 year ago

This looks great, I find the macros very readable and clear. Just to confirm:

* Using the `NEVER` or `ALWAYS` macros on a branch condition will properly exclude that branch from coverage?

Yes, you can find an example here: https://coveralls.io/builds/53603547/source?filename=src%2Fh3lib%2Flib%2FdirectedEdge.c#L282

  • The drop in coverage in this PR is due to the macros exposing reachable branches we had previously excluded from coverage? Yes, I had hoped this PR would increase coverage by being able to exclude some more branches we had not marked with exclusions, but rather it exposed they are reachable, and we should cover them via unit testing.
ajfriend commented 1 year ago

These macros are awesome!

ajfriend commented 1 year ago

@isaacbrodsky, this is maybe more of a conceptual question, but what if there were a user who was risk-tolerant but very interested in performance. Would it make sense for them to want to build a release version of the library that bypasses the defensive code (like we do in the coverage build)? Is that configuration possible, or should we allow for that?

Would that provide any potential performance benefit by skipping the defensive checks?

isaacbrodsky commented 1 year ago

@isaacbrodsky, this is maybe more of a conceptual question, but what if there were a user who was risk-tolerant but very interested in performance. Would it make sense for them to want to build a release version of the library that bypasses the defensive code (like we do in the coverage build)? Is that configuration possible, or should we allow for that?

Would that provide any potential performance benefit by skipping the defensive checks?

I don't think that configuration is supported here. It would be possible to eliminate some conditional checks, but I would imagine the performance gain to be modest. We could add another CMake option that controls whether those are included or not that's separate from (probably defaulted from) the coverage option.

ajfriend commented 1 year ago

I don't think that configuration is supported here. It would be possible to eliminate some conditional checks, but I would imagine the performance gain to be modest. We could add another CMake option that controls whether those are included or not that's separate from (probably defaulted from) the coverage option.

Makes sense. And I don't think we need to make any changes here. That was just a conceptual question.