Sonar fixes for Iex, IexTest, and ImathTest #575
Conversation
SonarCloud doesn't like floating-point variables in for loops. This spells out precisely what values are used in the loops. Signed-off-by: Cary Phillips <[email protected]>
Signed-off-by: Cary Phillips <[email protected]>
Signed-off-by: Cary Phillips <[email protected]>
| @@ -57,17 +57,17 @@ testFrustumPlanes (IMATH_INTERNAL_NAMESPACE::Frustumf &frustum) | |||
| IMATH_INTERNAL_NAMESPACE::V3f o (0.0f, 0.0f, 0.0f); | |||
| float eps = 5.0e-4; | |||
|
|
|||
| for (float xRo = 0.0f; xRo < 360.0f; xRo += 100.0f) | |||
| for (auto xRo : {0.0f, 100.0f, 200.0f}) | |||
There was a problem hiding this comment.
I guess you need auto instead of float to use this snazzy iteration syntax? It is cool...
There was a problem hiding this comment.
No you can still explicitly declare the type when using range-based for, so it's fine to declare 'float' here. Do we have a coding standard wrt to usage of auto? For UE4, our standard is that we always explicitly declare the type. The only exceptions where we use 'auto' are when declaring a variable set to a lambda, verbose iterator types, or in template code where the type cannot easily be determined. The advantage of auto's is preventing implicit conversion. But this is only beneficial when auto is used ubiquitously. Here being explicit by declaring the type float seems preferable.
There was a problem hiding this comment.
I don't think we've taken a stance on auto, but when we address it I would prefer the UE4 convention you describe.
There was a problem hiding this comment.
Sounds good, I leave it to Cary whether he wants to change this one or not.
There was a problem hiding this comment.
My vote is also for using the explicit type when it's something that is known and can be written compactly.
float x is better than auto x
But I totally respect auto x when it can replace an ugly mouthful like std:vector<std::pair<FooType,std::map<int>&>::const_iterator.
|
I have the same reaction towards auto, although I admit a certain irony in
saying use the type if the type is obvious (float) but use auto if the type
is something inscrutable and messy.
The downside to auto is it obscures the actual type you’re dealing with,
but that can be good or bad.
I just completed an exercise in my day job of changing a low level type
that’s used in other complex types - vectors of maps of pairs of shared
ptr’s, etc, and had to change a bunch of for loops in code I’m not familiar
with. I ended up changing them all to auto and it made the code a lot
easier to read, even though I’m not entirely sure what it does. This made
me a fan of auto.
So my vote is to use it liberally.
On Fri, Oct 4, 2019 at 3:01 PM Larry Gritz ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In IlmBase/ImathTest/testFrustum.cpp
<#575 (comment)>:
> @@ -57,17 +57,17 @@ testFrustumPlanes (IMATH_INTERNAL_NAMESPACE::Frustumf &frustum)
IMATH_INTERNAL_NAMESPACE::V3f o (0.0f, 0.0f, 0.0f);
float eps = 5.0e-4;
- for (float xRo = 0.0f; xRo < 360.0f; xRo += 100.0f)
+ for (auto xRo : {0.0f, 100.0f, 200.0f})
My vote is also for using the explicit type when it's something that is
known and can be written compactly.
float x is better than auto x
But I totally respect auto x when it can replace an ugly mouthful like
std:vector<std::pair<FooType,std::map<int>&>::const_iterator.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#575>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFC3DGIDKWEVOTSBBFWBFZDQM64LJANCNFSM4I45YKKA>
.
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
|
|
I tend to agree, auto within range based for is on my preferred list,
although I tend to prefer the native types when they aren't the looping
variable, unless inside a templated function of course. The only thing I
will caution against on the range-based for loops is that
for ( auto x: listlikething )
will incur a copy operator. This of course doesn't matter for float, but
does when it's a list of string, or similar "heavy" object. So I often do
for ( auto &x: thing )
or
for ( const auto &: thing )
if it's not obvious what's in the list to avoid copies...
K
On Sun, Oct 6, 2019 at 6:59 AM Cary Phillips <[email protected]>
wrote:
… I have the same reaction towards auto, although I admit a certain irony in
saying use the type if the type is obvious (float) but use auto if the type
is something inscrutable and messy.
The downside to auto is it obscures the actual type you’re dealing with,
but that can be good or bad.
I just completed an exercise in my day job of changing a low level type
that’s used in other complex types - vectors of maps of pairs of shared
ptr’s, etc, and had to change a bunch of for loops in code I’m not familiar
with. I ended up changing them all to auto and it made the code a lot
easier to read, even though I’m not entirely sure what it does. This made
me a fan of auto.
So my vote is to use it liberally.
On Fri, Oct 4, 2019 at 3:01 PM Larry Gritz ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In IlmBase/ImathTest/testFrustum.cpp
> <#575 (comment)>:
>
> > @@ -57,17 +57,17 @@ testFrustumPlanes
(IMATH_INTERNAL_NAMESPACE::Frustumf &frustum)
> IMATH_INTERNAL_NAMESPACE::V3f o (0.0f, 0.0f, 0.0f);
> float eps = 5.0e-4;
>
> - for (float xRo = 0.0f; xRo < 360.0f; xRo += 100.0f)
> + for (auto xRo : {0.0f, 100.0f, 200.0f})
>
> My vote is also for using the explicit type when it's something that is
> known and can be written compactly.
>
> float x is better than auto x
>
> But I totally respect auto x when it can replace an ugly mouthful like
> std:vector<std::pair<FooType,std::map<int>&>::const_iterator.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <
#575
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AFC3DGIDKWEVOTSBBFWBFZDQM64LJANCNFSM4I45YKKA
>
> .
>
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#575>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHM3IHOYU6QFD6PJD4OSALQNDIXXANCNFSM4I45YKKA>
.
|
|
Totally agree with both. In any kind of range-for based on a container class, I always use I think this particular idiom of |
Various fixes to address SonarCloud "bugs":