Out of bounds `last` argument to assign_vgl, assign_vgh, assign_vghgh silently fixed.
- Dominant language
- C++
- Stars
- 403
- Forks
- 154
- Avg merge
- 1d 12h
- Merged PRs (30d)
- 82
Description
**Describe the smell**
```c++
inline void SplineR2R::assign_vgl(int bc_sign,
ValueVector& psi,
GradVector& dpsi,
ValueVector& d2psi,
int first,
int last) const
{
// protect last
last = last > kPoints.size() ? kPoints.size() : last;
...
```
This "feature" violates the semantics of all these methods. Even though last is undocumented it is clearly the bound of assigned values. Why does it need to take invalid values and be silently fixed? Defensive programming like this indicates something stinks.
The Proximate cause is Spline classes make myV,myL,myG,... larger than they are, padded out to the end of the alignment boundary. So its "the padding issue."
So when later code uses that size instead of the actual number of elements (which is not actually known at the container scope) FairDivideAligned is given an ntot argument which is not the number of elements but the size of the workspace. The fixup of last in the assigns is actually the return of the real numbers of V's G's, etc. It must be recovered from the `size()` of the kPoints vector in the base class 😭
spline2 seems to be the source of the alignment requirements. But it shouldn't necessitate inconsistent use and semantics throughout the code.
Is this where it started?
My suggestion is to use a type for `myV` that doesn't violate the semantics of the `size()` method and allocates up to the alignment boundary. Spline2 continues to get pointers to "data" but does not get size but `capacity()` which is a name that carries the correct semantics. Other code returns to sensible size semantics.
As far as I can see we actually use this call chain with Ohmms containers. `Vector` is just missing the`capacity()` call and a unit test that enforces that access through the data ptr is legal behavior all the way to the end of nAllocated.
This somewhat isolated smell in the splines could be cleaned up.
More generally the attachReference behavior of the OhmmsVector complicates implicit padding and standard `size()` semantics. I think this is because nAllocated is doing too much work in that class. And would be better replaced by a `size_t n_capacity_` and a `bool mem_owned_`
**Additional context**
Code cov seemed to be having a bit of a meltdown over these defensive lines so I followed up.
Contributor guide
Assessment
This issue has not been assessed yet.