Created attachment 362767 [details] [review]
ring: Silence -Wunsafe-loop-optimizations

Created attachment 362768 [details] [review]
ring: Silence -Wunused-variable and -Wunused-but-set-variable

Thanks; there are like a dozen or so warnings while compiling vte, it would be nice to fix all of them.

As for the first patch, though: I'd much rather have a warning than incorrect semantics, i.e. the variable "idx" meaning "one less than the index", it's misleading and error prone. Your patch leaves "return idx;" intact, so I'm quite certain you've also fallen for this and the patch is incorrect.

If you insist, I think the variable should be renamed to "idx_minus_one" or similar. But I'd prefer to better understand the situation to see if a nicer solution is available. Is perhaps changing the condition from "idx <= highest" to "idx < highest + 1" good enough?

As for the second: I'm fine with it (I didn't know about this trick); alternatively you could go for conditional declaration of the variable, or even remove it altogether (also from the debug messages). These are variables with names that probably improve readability of the code even if they're unused, so probably let's just stick with your patch, thanks! :-)

(In reply to Egmont Koblinger from comment #3)
> As for the first patch, though: I'd much rather have a warning than
> incorrect semantics, i.e. the variable "idx" meaning "one less than the
> index", it's misleading and error prone. Your patch leaves "return idx;"
> intact, so I'm quite certain you've also fallen for this and the patch is
> incorrect.

Oh, yeah, I don't know how I missed that return statement. I tried a few other approaches like using a while loop before setting for this version, so I missed it while iterating on it.

> If you insist, I think the variable should be renamed to "idx_minus_one" or
> similar. But I'd prefer to better understand the situation to see if a nicer
> solution is available. Is perhaps changing the condition from "idx <=
> highest" to "idx < highest + 1" good enough?

"idx < highest + 1" would still be unsafe because highest + 1 can still overflow past the type's maximum limit. At least that's my understanding of the original warning. ie. i++ will overflow if hyperlink_highest_used_idx is at the maximum possible value.

The other option is to use a pragma and suppress the warning.

(In reply to Debarshi Ray from comment #4)

> "idx < highest + 1" would still be unsafe because highest + 1 can still
> overflow past the type's maximum limit. At least that's my understanding of
> the original warning. ie. i++ will overflow if hyperlink_highest_used_idx is
> at the maximum possible value.

My understanding:

It doesn't matter what's at the right side. A +1 or lack thereof doesn't make a difference. For what it's worth, another variable "highest_plus_one = highest + 1" could be defined in the previous line, and then there'd be no "+1" in the for loop's condition.

What does make a significant difference is "<" vs "<=". With "<" the loop counter cannot overflow. Whatever the right hand side is, the loop terminates no later than the left hand side reaching its maximum value, overflow is not possible. With "<=" an overflow and even an infinite loop is possible. (This is all for the case when the upper limit doesn't change and the loop counter increases by 1.)

Signed vs. unsigned data types can possibly also make a difference.

What I'm totally puzzled about, and would be curious to understand, is why does this very same for loop line compile without warning in line 187, and then gives this warning in line 247.

> The other option is to use a pragma and suppress the warning.

Yup, that's another option. I mean, gcc would still miss the optimization, but wouldn't warn us. Maybe "< ... +1" is a cleaner better one.

(In reply to Egmont Koblinger from comment #5)
> (In reply to Debarshi Ray from comment #4)
> 
> > "idx < highest + 1" would still be unsafe because highest + 1 can still
> > overflow past the type's maximum limit. At least that's my understanding of
> > the original warning. ie. i++ will overflow if hyperlink_highest_used_idx is
> > at the maximum possible value.
> 
> My understanding:
> 
> It doesn't matter what's at the right side. A +1 or lack thereof doesn't
> make a difference. For what it's worth, another variable "highest_plus_one =
> highest + 1" could be defined in the previous line, and then there'd be no
> "+1" in the for loop's condition.

Oh, yes, that's right because an overflowing "highest + 1" won't be bigger than idx and will break the loop.

> What I'm totally puzzled about, and would be curious to understand, is why
> does this very same for loop line compile without warning in line 187, and
> then gives this warning in line 247.

I guess we'll leave it unanswered.

My recommendation, if you're fine with it too, is to go for the "< ... + 1" version at both occurrences (even the one that doesn't give a warning, to make the source more consistent).

Thanks!

Created attachment 366728 [details] [review]
ring: Silence -Wunsafe-loop-optimizations

I'd pull the + 1 out of the loop, so

auto const last_idx = ... + 1;
for ( idx = 1; idx < last_idx; ++idx)
  ...

I too am curious about the other loop (comment 5) being ok :-)

We're now in hard code freeze, but this is good to go in right after freeze lifts.

(In reply to Christian Persch from comment #9)
> I'd pull the + 1 out of the loop, so
> 
> auto const last_idx = ... + 1;
> for ( idx = 1; idx < last_idx; ++idx)
>   ...

Ok.

By the way, did you intend to mark attachment 362768 [details] [review] as accepted-commit_after_freeze instead?

Created attachment 369660 [details] [review]
ring: Silence -Wunsafe-loop-optimizations

Comment on attachment 369660 [details] [review]
ring: Silence -Wunsafe-loop-optimizations

Pushed to master.

Created attachment 370477 [details] [review]
parser: Ignore -Wimplicit-fallthrough

Thanks for the review, chpe!

Any thoughts on attachment 362768 [details] [review] ? There are a few other places -Wunused-variable and -Wunused-but-set-variable get flagged. So I am wondering if we should use the same approach to silence them.

Comment on attachment 370477 [details] [review]
parser: Ignore -Wimplicit-fallthrough

Ok as a temporary fix.

We'll require C++17 soon (as soon as I can upgrade to a system with GCC 7), then we can use [[fallthrough]] attribute here.

Thanks!

Comment on attachment 370477 [details] [review]
parser: Ignore -Wimplicit-fallthrough

Pushed to master.

Ping (re: attachment 362768 [details] [review])

Comment on attachment 362768 [details] [review]
ring: Silence -Wunused-variable and -Wunused-but-set-variable

Since these 2 variables are used only in the debug output, maybe just remove them and replace them inside the vte_debug_print with the value they were set to. Let's go with that easy fix, since I don't like working around warnings like that.

An alternative would be copying mozilla's DebugOnly<> template.

I noticed that the warnings seem to have disappeared. Latest Git snapshots don't emit any.

Let's close this bug.