Type conversion for hundreds of lines

[JiiPee]

I have been pondering this many times. I keep it short:
If I have hundreds of lines like this:

short a;
std::vector<int> v;
...
a = v.size();

This gives a warning: "warning, assigning size_t to short".
I know this can be fixed:
a = static_cast<short>(v.size());

but if we have hundreds of those lines, how would you fix this? Place a
static cast in all of them? Of create some helper funktion to do this?

How about if I have a function created by me before:
size_t CalcLength(int a);

and then I call it 100 places like this:
short b;
b = CalcLength(466);

again type warning. Would you fix it by placing static cast in 100
places like this:

b = static_cast<short>(CalcLength(466));

wherever this cast can be done? And what if CalcLength() can in some
code places return a large number also? So we cannot just blindly change
the return type to "short" to solve this.

[JiiPee]

... also, how to you guys do this kind of situation:

int sum = 0;
for(int i = v.size() - 1; i >= 0; --i)
sum += i;

..so assuming we cannot change the type of i.

again types different, so would you do:
for(int i = static_cast<int>(v.size()) - 1; i >= 0; --i)
?

there are alot of place where vectors size() is assigned to an int. So
you always use static cast there?

[JiiPee]

On 12/10/2022 23:54, JiiPee wrote:
> ... also, how to you guys do this kind of situation:
>
> int sum  = 0;
> for(int i = v.size() - 1; i >= 0; --i)
>     sum  += i;
>

forgot to add that v is std::vector<int>.

[Paavo Helde]

Why on earth are you having local variables of type short?

I would fix this ASAP either by s/short/auto/ or s/short/size_t/.

[Scott Lurndal]

JiiPee <kerrttuPo...@gmail.com> writes:
>I have been pondering this many times. I keep it short:
>If I have hundreds of lines like this:
>
>short a;
>std::vector<int> v;
>...
>a = v.size();
>
>This gives a warning: "warning, assigning size_t to short".
>I know this can be fixed:
>a = static_cast<short>(v.size());

so declare 'a' as size_t. Problem fixed.

[Keith Thompson]

JiiPee <kerrttuPo...@gmail.com> writes:
> I have been pondering this many times. I keep it short:
> If I have hundreds of lines like this:
>
> short a;
> std::vector<int> v;
> ...
> a = v.size();
>
> This gives a warning: "warning, assigning size_t to short".
> I know this can be fixed:
> a = static_cast<short>(v.size());
>
> but if we have hundreds of those lines, how would you fix this? Place
> a static cast in all of them? Of create some helper funktion to do
> this?

Why is `a` defined as a short and not as a size_t?

If there's a very good reason that `a` *needs* to be a short, it makes
sense to consider some kind of cast. If not, just make it a size_t.

--
Keith Thompson (The_Other_Keith) Keith.S.T...@gmail.com
Working, but not speaking, for Philips
void Void(void) { Void(); } /* The recursive call of the void */

[Paavo Helde]

int is a wrong type here. A correct one would be std::ptrdiff_t.

Anyway, even if I were convinced that int would be large enough in all
imagined circumstances, I still wouldn't use static_cast here, but e.g.
boost::numeric_cast, or at least my own debug_cast, which resolves to
boost::numeric_cast in debug builds and static_cast in release builds.

Note: boost::numeric_cast will throw an exception in case my reasoning
was wrong, which is a very good thing.

[Ben Bacarisse]

... and presumably you meant sum += v[i]; or the loop is pointless!

And why run the indexes backwards? It might be needed for some vectors
of floating-point numbers, but not for int.

It seems that a lot of peculiar choices have been made in the code base.

--
Ben.

[JiiPee]

But if its old code and hundreds of places, that would require a lot of
testing, right? But you would still change it even though might cause
other issues?

[JiiPee]

but if in hundreds of places, that would cause othe problems... and
needs a lot of testing? you would still do it? but the probram should be
then tested and check it does not cause other side issues.

[JiiPee]

On 13/10/2022 00:30, Keith Thompson wrote:
> Why is `a` defined as a short and not as a size_t?
>
> If there's a very good reason that `a`*needs* to be a short, it makes
> sense to consider some kind of cast. If not, just make it a size_t.

Lets assume its some old code... for example from 80's... and you have
it now. Would you do this change to hundreds of places, to change a to
size_t? But that might cause sides issues, isnt it? What if the other
code is relying on the short, and for example takes sizeof() of the
short when storing to a file.

[JiiPee]

On 13/10/2022 01:26, Ben Bacarisse wrote:
> ... and presumably you meant sum += v[i]; or the loop is pointless!
>
> And why run the indexes backwards? It might be needed for some vectors
> of floating-point numbers, but not for int.
>
> It seems that a lot of peculiar choices have been made in the code base.

the example is only created to illustrate the copy propblem... its not
an example from a real code.

The point is, that if I copy in a for-loop vector::size() to an integer,
do you always do the casting for it? Because there are many places that
can happen....

[Chris M. Thomasson]

Is the code busted as-is using a short? Do you actually _need_ to change
short to size_t? Warnings aside for a moment...

[JiiPee]

On 13/10/2022 00:37, Paavo Helde wrote:
> wouldn't use static_cast here, but e.g. boost::numeric_cast,

OK, interesting option.

Ok, but you would always cast it somehow and not for example create some
conversion function etc?

I mean, if this conversion is needed in 500 places, you would just add
that boost::numeric_cast (or some other cast) into these places?

It makes the code quite "long" and a little difficult to read if adding
a lot of casts... but maybe its the only way.

[JiiPee]

On 13/10/2022 07:30, Chris M. Thomasson wrote:
> Do you actually _need_ to change short to size_t? Warnings aside for a
> moment...

Good question. No, not necessarily... its only a warning in a compiler.
But.. obviously if the compiler is warning alot then better to check all
those warnings, isnt it? At least check all of them... but are you
saying I do not need to change anything, just leave the warnings there?
Just check the code, and if its OK then just not minding the warnings
rather than change hundreds of places?

[Juha Nieminen]

JiiPee <kerrttuPo...@gmail.com> wrote:
> I have been pondering this many times. I keep it short:
> If I have hundreds of lines like this:
>
> short a;
> std::vector<int> v;
> ...
> a = v.size();
>
> This gives a warning: "warning, assigning size_t to short".
> I know this can be fixed:
> a = static_cast<short>(v.size());
>
> but if we have hundreds of those lines, how would you fix this? Place a
> static cast in all of them? Of create some helper funktion to do this?

Explicitly saying "static_cast<short>(...)" kind of indicates that you
are saying "yes, I'm fully aware that the value inside the parentheses
technically speaking may be larger than fits in a short, but I know
that it never will in this particular code, so this assignment is done
intentionally and it's not just an oversight (and yes, I am doing this
even knowing the risk that it could potentially cause a bug in the
future)".

So, in a manner of speaking, it's self-documenting code. Thus, I would
use that.

However, if you want to safeguard against that possible future bug,
you could use a function instead, and add a check in the function
(for example an assert(), or possibly a throw), unless this is a
very time-critical code (which I assume it isn't).

[Paavo Helde]

13.10.2022 07:34 JiiPee kirjutas:
> On 13/10/2022 00:37, Paavo Helde wrote:
>> wouldn't use static_cast here, but e.g. boost::numeric_cast,
>
> OK, interesting option.
>
> Ok, but you would always cast it somehow and not for example create some
> conversion function etc?
>
> I mean, if this conversion is needed in 500 places, you would just add
> that boost::numeric_cast (or some other cast) into these places?

boost::numeric_cast is technically a function, not a cast.

>
> It makes the code quite "long" and a little difficult to read if adding
> a lot of casts... but maybe its the only way.

Right. In real code I'm using my own checked_cast and debug_cast, which
names I consider reasonable readable and greppable. These are a bit
involved because the boost::numeric_cast did not always quite do what we
wanted exactly, but for brevity I leave out these details from here.

template<typename T, typename U>
inline constexpr T checked_cast(U x) {
// Some code to avoid false alarms on MS "smaller type check.
// ...
// Some code to refuse converting NaN to uint64
// ...
return boost::numeric_cast<T, U>(x);
}

// A specialization for double->float conversion
// to avoid false alarm on -inf, and to allow converting
// large finite double values to float inf.
template<>
inline constexpr float checked_cast<float, double>(double x) {
return static_cast<float>(x);
}

template<typename T, typename U>
inline constexpr T debug_cast(U x) {
#ifdef NDEBUG
return static_cast<T>(x);
#else
return checked_cast<T,U>(x);
#endif
}


I'm using debug_cast whenever I am quite sure the cast should always
work, and checked_cast if there is any doubt.

[Paavo Helde]

If it's old code without unit tests, then one should start with writing
tests.

When the code gets covered by tests enough, only then it becomes
possible to make major code refactorings.

Not sure if fixing short would qualify as a major refactoring or not. I
understand that you want to get rid of warnings. It means you have to
change all those hundreds of places anyway, so why not fix it properly
instead of hiding the potential problems with a cast?

The warning is there for a reason. and hiding it with a cast might just
hide a bug.

In general it should be not so tricky to fix vec.size() result from
short to size_t or auto. One just needs to inspect the code to see that
the value never becomes negative. In my experience old dirty codebases
contain a lot of copy-paste, so it should be possible to recognize the
patterns quite soon. Of course, a proper refactoring would get rid of
copy-pasted code, and I bet a lot of those 'short' variables could be
just eliminated by switching over to range-based for loops.

[David Brown]

With old code that has techniques that you consider questionable today,
there are usually three options.

One is to consider the code as "tried and tested" and assume it is
correct. Add warning disable pragmas at the start of the code to
minimise noise. Obviously the details here depend on the compiler and
the warnings - as an example, you might have :

#pragma GCC diagnostic ignored "-Wsign-conversion"

Clearly this is a dangerous path - hiding the potential problems. But
maybe the code works fine in practice, as many compiler warnings are
about /potential/ problems rather than real ones.


Number two is to pick compiler options that change the semantics of the
language to match the code's assumptions. A common example is to have
"-fwrapv" (or a GCC/clang/icc pragma - it's often better to have a
pragma in the code than a compiler flag) to turn old code that assumes
wrapping integer overflow into correct code. This only applies to
certain cases, however.


Number three is to fix the code. That might mean changing the types of
variables, refactorising, modernising, or otherwise changing the code.
I'd be sceptical about simply adding static casts - the suggestion of
using a function that supports optional run-time checks is probably better.

[Ben Bacarisse]

JiiPee <kerrttuPo...@gmail.com> writes:

> On 13/10/2022 01:26, Ben Bacarisse wrote:
>> ... and presumably you meant sum += v[i]; or the loop is pointless!
>> And why run the indexes backwards? It might be needed for some vectors
>> of floating-point numbers, but not for int.
>> It seems that a lot of peculiar choices have been made in the code base.
>
> the example is only created to illustrate the copy propblem... its not
> an example from a real code.

Sure, but the loop running backwards, starting at size-1, introduces other
things to be careful about.

> The point is, that if I copy in a for-loop vector::size() to an
> integer, do you always do the casting for it?

No. Since the scope is small in a for loop, I'd change the declaration
and run the loop forward. It's more changes, but it will make the code
clearer.

If I had reliable regression tests, I'd consider writing

int sum = std::accumulate(v.begin(), v.end(), 0);

> Because there are many places that can happen....

Roughly how many? I think it makes sense to tidy up the code rather
than spray it with casts.

--
Ben.

[JiiPee]

On 13/10/2022 12:00, Paavo Helde wrote:
> If it's old code without unit tests, then one should start with writing
> tests.
>
> When the code gets covered by tests enough, only then it becomes
> possible to make major code refactorings.

ye I agree, best way if possible to make tests to be sure changes will
not harm other code.

>
> Not sure if fixing short would qualify as a major refactoring or not. I
> understand that you want to get rid of warnings. It means you have to
> change all those hundreds of places anyway, so why not fix it properly
> instead of hiding the potential problems with a cast?

hmm, fair point. Especially if those places are not too many, like only
40 for example.

[JiiPee]

On 13/10/2022 12:17, David Brown wrote:
> One is to consider the code as "tried and tested" and assume it is
> correct.  Add warning disable pragmas at the start of the code to
> minimise noise.  Obviously the details here depend on the compiler and
> the warnings - as an example, you might have :
>
>     #pragma GCC diagnostic ignored "-Wsign-conversion"

good point, in old code can assume if its well tested.
Can I disable a warning blockwise or filewise in Visual Studio?

>
> Clearly this is a dangerous path - hiding the potential problems.  But
> maybe the code works fine in practice, as many compiler warnings are
> about /potential/ problems rather than real ones.

yes, if its for example tested 20 years....

>
>
>
> Number three is to fix the code.  That might mean changing the types of
> variables, refactorising, modernising, or otherwise changing the code.
> I'd be sceptical about simply adding static casts - the suggestion of
> using a function that supports optional run-time checks is probably better.

good point... rather use more sophisticated conversion function than cast.

[JiiPee]

On 13/10/2022 10:52, Juha Nieminen wrote:
> So, in a manner of speaking, it's self-documenting code. Thus, I would
> use that.

I was thinking the same.

>
> However, if you want to safeguard against that possible future bug,
> you could use a function instead, and add a check in the function
> (for example an assert(), or possibly a throw), unless this is a
> very time-critical code (which I assume it isn't).

yes like other also said, function more accurate. So Currently I kinda
lean on the function solution.

[Paavo Helde]

13.10.2022 20:32 JiiPee kirjutas:
> Can I disable a warning blockwise or filewise in Visual Studio?

MSVC also has pragmas to suppress warnings, with different syntax. Example:

#ifdef _MSC_VER
#pragma warning(disable:4100) // unreferenced formal parameter
#endif

[JiiPee]

On 13/10/2022 21:13, Paavo Helde wrote:
> MSVC also has pragmas to suppress warnings, with different syntax. Example:
>
> #ifdef _MSC_VER
> #pragma warning(disable:4100)    // unreferenced formal parameter
> #endif

ok thanks. so with this I can switch it on and off

[David Brown]

On 13/10/2022 19:32, JiiPee wrote:
> On 13/10/2022 12:17, David Brown wrote:
>> One is to consider the code as "tried and tested" and assume it is
>> correct.  Add warning disable pragmas at the start of the code to
>> minimise noise.  Obviously the details here depend on the compiler and
>> the warnings - as an example, you might have :
>>
>>      #pragma GCC diagnostic ignored "-Wsign-conversion"
>
> good point, in old code can assume if its well tested.
> Can I disable a warning blockwise or filewise in Visual Studio?
>

Not a clue, sorry. I'd assume there are equivalent pragmas, but there's
no point in my googling for you!

>>
>> Clearly this is a dangerous path - hiding the potential problems.  But
>> maybe the code works fine in practice, as many compiler warnings are
>> about /potential/ problems rather than real ones.
>
> yes, if its for example tested 20 years....
>

Of course, you can only be sure it works if your compiler today doesn't
do any optimisations or different code generation techniques from the
compiler of 20 years ago.

(Sometimes I work with projects that were written and tested up to 20+
years ago - but I archive the compiler along with the project, so that
the generated binary is identical.)

[Chris M. Thomasson]

Well, is using short an actual source of real bugs in the original code
base? Please, define "not necessarily"? I was under the impression that
this is "legacy" code, so to speak. Your compiler might have a way to
suppress certain warnings. Let's say, you know that the code works
as-is, period. So, the warnings are not worth your time. Therefore,
artificially suppress them.

[JiiPee]

On 14/10/2022 00:18, Chris M. Thomasson wrote:
> So, the warnings are not worth your time. Therefore, artificially
> suppress them.

yes this is something to consider.
Then in new code can focus making it warning free.

"Please, define "not necessarily"?"

I mean, likely the code does not need change. so its only a warning
coming but does not need action.

[Mike Terry]

On 13/10/2022 18:32, JiiPee wrote:
> On 13/10/2022 12:17, David Brown wrote:
>> One is to consider the code as "tried and tested" and assume it is correct.  Add warning disable
>> pragmas at the start of the code to minimise noise.  Obviously the details here depend on the
>> compiler and the warnings - as an example, you might have :
>>
>>      #pragma GCC diagnostic ignored "-Wsign-conversion"
>
> good point, in old code can assume if its well tested.
> Can I disable a warning blockwise or filewise in Visual Studio?

You can use

#pragma warning( push )

and

#pragma warning( pop )

to save/restore the current warning settings. E.g. a header file could be structured:

---- start header ----
#pragma warning( push )
#pragma warning( disable : 4706 )
...
...
#pragma warning( pop )
---- end header ----

to disable warning 4706 for just that header file.

Mike.

[JiiPee]

On 14/10/2022 00:36, Mike Terry wrote:
> You can use
>
> #pragma warning( push )
>
> and
>
> #pragma warning( pop )
>
> to save/restore the current warning settings.

thanks. I ll save this and can try it :).

[Chris M. Thomasson]

Can you isolate the code in a shared or static library with an API
interface and label it version_original or something? Perhaps, stuff it
under its own namespace?

There are some tricks wrt introducing new functions on existing code in C.

[Frederick Virchanza Gotham]

On Wednesday, October 12, 2022 at 9:49:01 PM UTC+1, JiiPee wrote:

> This gives a warning: "warning, assigning size_t to short".
> I know this can be fixed:
> a = static_cast<short>(v.size());
>
> but if we have hundreds of those lines, how would you fix this? Place a
> static cast in all of them? Of create some helper funktion to do this?

Try this regex:

https://regex101.com/r/5tsXx3/1