Cleanup additional warnings #407
Conversation
| @@ -630,7 +630,7 @@ template <class ElementType, std::ptrdiff_t Extent> | |||
| constexpr ElementType& at(const span<ElementType, Extent>& s, size_t index) | |||
| { | |||
| // No bounds checking here because it is done in span::operator[] called below | |||
| return s[index]; | |||
| return s[narrow_cast<typename span<ElementType, Extent>::index_type>(index)]; | |||
There was a problem hiding this comment.
This is the only one that I was not sure about what to do as we could use gsl::narrow or gsl::narrow_cast. I used gsl::narrow_cast as I know a lot of people have brought up issues with performance so I figured that would be the likely choice.
There was a problem hiding this comment.
I think you're fine to use gsl::narrow_cast() here because you know that s will validate the bounds anyway. So I like it as you have it.
There was a problem hiding this comment.
Since the other PR changes the index type to std::ptrdiff_t, there is no need for this cast anymore. Once the other PR is merged, I will clean this one up so that only the fixes that are needed there.
|
|
||
| for (int i = 0; i < 4; ++i) | ||
| for (size_t i = 0; i < 4; ++i) | ||
| CHECK(at(a, i) == i+1); |
There was a problem hiding this comment.
at takes a size_t so attempting to put an int in it will result in a signed conversion warning.
There was a problem hiding this comment.
Hmm, I missed that. Why is at() taking a different type than []?
There was a problem hiding this comment.
You tell me 😄
That is the one that's been there for a while (this is testing gsl::at and not the new gsl::span::at).
There was a problem hiding this comment.
Yes; it was question to "us". It is interesting the compiler isn't warning about i < 4, presumably because 4 is small enough?
There was a problem hiding this comment.
I'll test with Clang, but I'm almost positive it will complain about the same thing as I have had to make a lot of changes like this to support Clang Tidy with these warnings enabled
There was a problem hiding this comment.
Yes, clang has the same warning (and some others I might add, but we can address those in a different PR)
/home/user/gsl/tests/at_tests.cpp:62:25: warning: implicit conversion changes signedness: 'int' to 'size_t' (aka 'unsigned long')
[-Wsign-conversion]
CHECK(at(a, i) == i+1);
There was a problem hiding this comment.
I'm with @gdr-at-ms ... why does at() take size_t? It should not. It should take ptrdiff_t. If that's my fault, it's probably because we never updated it after the original prototype back in the mists of the past. But that should change, rather than be worked around.
There was a problem hiding this comment.
@neilmacintosh ok I can update the PR to change this.
There was a problem hiding this comment.
Thanks @rianquinn and thanks for #408 which corrects my laziness!
|
I'd like to work on the following PR first, before we continue with this one. Once we address the issue with |
When turning on the following flags, several additional warnings were generated, which have been cleaned up in this patch. The flags included: -Wextra -Wpedantic -Wconversion -Wsign-conversion -Wctor-dtor-privacy -Wshadow -Wnon-virtual-dtor -Wold-style-cast -Wcast-align -Woverloaded-virtual Signed-off-by: “Rian <“rianquinn@gmail.com”>
rianquinn
force-pushed
the
additional_compiler_warnings
branch
from
bdfdb75
to
2f7b173
Compare
|
@neilmacintosh I updated this PR now that the |
|
@rianquinn thanks very much for doing this useful cleanup! It looks great to me. |
When turning on the following flags, several additional warnings
were generated, which have been cleaned up in this patch. The
flags included:
-Wextra
-Wpedantic
-Wconversion
-Wsign-conversion
-Wctor-dtor-privacy
-Wshadow
-Wnon-virtual-dtor
-Wold-style-cast
-Wcast-align
-Woverloaded-virtual
Signed-off-by: “Rian <“rianquinn@gmail.com”>