Pessimization Concerns

[daviddetweiler]

(Preface: the Microsoft implementation of the GSL is by far the most visible / vigorously maintained version, so when I refer to "the GSL", I refer to this implementation)

I think we can agree that the general spirit of the guidelines is to ensure safety / correctness efficiently (hilosphy section claims as much; including avoiding "redundant checks"). I've seen two places where the last concern might not have been applied so well. Part of this might very well be an issue specifically with the GSL implementation, but the guidelines as of the last few days seem to more-or-less encourage these issues.

1. Expects()/Ensures() being pessimized choices for contract statement. The rules encouraging Expects()/Ensures() are themselves fine: expose contracts directly to tooling, provide opportunity for runtime enforcement, etc. But the current specification of their behavior is that they terminate if false. Suddenly all contract checks (which these guidelines encourage to be used very frequently) "do something". Sure, the optimizer can probably see through the check in simple cases, but it just as often can't in the more complicated cases (MSVC in particular seems particularly conservative when reasoning about when a condition can/cannot be always true/false). It seems extremely pessimistic to ask for a generally difficult to optimize runtime check to be injected at every place an interface its used, considering how often the usage is statically correct. The GSL used to have support for essentially-free contracts (__assume(condition), etc.), removed them some time ago, and I have seen at least one stale PR in the GSL that aimed to reintroduce this configurability. This has ramifications for span and not_null as well, as both use these liberally.

2. gsl::span and gsl::not_null. The latter's issue is simple: for plain pointers, assuming the correctness of not_null itself, the Expects(/* inner wrapped pointer */ != nullptr) found in not_null::get() is unnecessary. The justification that the Microsoft implementation gives is a) this is a documentation of the contract b) this check isn't unnecessary for some unique_ptr edge cases. Once again, asking for this check to be enabled for all cases where only one case actually needs it, when Expects() "does something", seems a pessimization. As for gsl::span, I refer to the mandatory bounds-checking of its iterators. Take the logical equivalent of the ranged for-loop:

   auto iter = my_span.begin();
   const auto end = my_span.end();
   for (; iter != end; ++iter)
   do_something(*iter);

While clang and gcc are able to completely see through the entire loop, MSVC completely fails to eliminate at least one per-loop check at /O2. The GSL implementers have taken to using the undocumented _Unchecked_begin and _Unchecked_end hooks to prevent otherwise grossly suboptimal code in the for (auto& thing : span) {} case. Now, if gsl::span is designed to pessimistically check all iterator operations and terminate on violations, I don't think it should be recommended as the kosher general-use alternative. std::span made the right choice, in my opinion, by not requiring iterator validation, while leaving it open for such validation to be configured (similar to debug iterators in the MSVC STL). If it is instead, like the not_null case, meant to a documentation of its contract, we fall back to the issue with the pessimized behavior of Ensure()/Expect().

(In fact, the LLVM version that ships as a drop-in replacement for MSVC in visual studio also seems to have issues in the simple case above)

I apologize if this is very long-winded issue; I have thought about this issue for some time now and have tried, as best as I can, to find the causes in both the guidelines and implementation decisions for the current state of affairs.

[daviddetweiler]

I'm also not the first person to have these concerns: https://github.com/isocpp/CppCoreGuidelines/issues/1561#issuecomment-616486172. There are four scenarios of "intended behaviors" I can think of:

1. Expects() is a pure contract annotation (and is not required to do anything at runtime), and gsl::span is a std::span equivalent (bounds-safety is part of its contract, but is not required to be checked). The implication for Microsoft's GSL: pretty much none, except for a change to the guidelines to remove the current requirement that Expects()/Ensures() terminate on false.
2. Expects() is a runtime contract check and gsl::span ensures at runtime that its bounds-safety is enforced. Changes: none, this is what happens right now, and the Microsoft implementation has already justified on basis of the current guidelines.
3. Expects() is a pure contract annotation and gsl::span ensures bounds-safety at runtime; changes to the guidelines: span probably shouldn't be the kosher suggestion the guidelines imply it is, as it is, by definition, pessimistic.
4. Expects() is a runtime contract check and gsl::span does not do runtime checks; this possibility I don't think is even a remotely reasonable interpretation of the intent of the guidelines, as the "spirit" of span is ultimately tied to that of Expects.

The question ultimately comes down to what is intended by the editors; as the guidelines are somewhat vague and possibly overly pessimistic.

[MikeGitb]

I think we can agree that the general spirit of the guidelines is to ensure safety / correctness efficiently (hilosphy section claims as much; including avoiding "redundant checks").

Just saying: "efficiently" is not the same as "at zero (runtime) cost".

[cubbimew]

Editor's meeting: we looked at this in the past and backed off pending committee decision regarding Contracts, but we will take another look.