Fix compiler warnings #329

SFrijters · 2020-04-20T14:58:41Z

Description

Attempts to fix #327 .

I've tried to restructure my changes into separate commits to address separate issues for easier reviewing My commit c57a0bd in particular is rather opinionated, so that one probably warrants extra attention.

In general I'm trying to make the compilers as happy as possible by making actual changes, but some warnings have been suppressed explicitly.

This has been tested with Intel 2019, GCC 8.3 and Clang 9.

codecov · 2020-04-20T15:08:28Z

Codecov Report

Merging #329 into master will decrease coverage by 0.08%.
The diff coverage is 57.79%.

@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
- Coverage   75.30%   75.21%   -0.09%     
==========================================
  Files         281      282       +1     
  Lines       19453    19493      +40     
==========================================
+ Hits        14649    14662      +13     
- Misses       4804     4831      +27

Impacted Files	Coverage Δ
include/occa/lang/primitive.hpp	`43.58% <ø> (ø)`
include/occa/tools/testing.hpp	`55.55% <ø> (ø)`
include/occa/tools/vector.hpp	`100.00% <ø> (ø)`
src/core/kernel.cpp	`89.52% <0.00%> (ø)`
src/tools/vector.cpp	`52.00% <33.33%> (-48.00%)`	⬇️
src/lang/primitive.cpp	`25.00% <34.61%> (ø)`
src/tools/testing.cpp	`58.62% <47.82%> (-41.38%)`	⬇️
src/c/types.cpp	`83.65% <71.42%> (-0.49%)`	⬇️
src/lang/modes/oklForStatement.cpp	`86.45% <87.50%> (ø)`
include/occa/core/scope.hpp	`100.00% <100.00%> (ø)`
... and 12 more

noelchalmers · 2020-04-21T17:19:27Z

For some reason with GCC 5.4 the fix to suppress the OpenCL polyfill unused variables doesn't seem to work. Still spits a bunch of warnings.

SFrijters · 2020-04-22T09:28:00Z

Hm, looks like that might be a compiler bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69967
Fixed in 6.x, but not in 5.4.

SFrijters · 2020-04-22T09:54:11Z

Rebased on new master and changed new function isBitwiseEqual to areBitwiseEqual for more consistency with the existing family of areEqual functions.

dmed256 · 2020-04-22T15:27:50Z

@SFrijters Cool thanks, I'll take a look at the code changes tonight!

dmed256

Aside from removing some of the primitive methods, most of the changes look really good. Thanks for fixing these compiler warnings!

dmed256 · 2020-05-22T03:59:33Z

src/lang/primitive.cpp

@@ -248,8 +249,8 @@ namespace occa {
    case primitiveType::uint32_ : return primitive(!p.value.uint32_);
    case primitiveType::int64_  : return primitive(!p.value.int64_);
    case primitiveType::uint64_ : return primitive(!p.value.uint64_);
-    case primitiveType::float_  : return primitive(!p.value.float_);
-    case primitiveType::double_ : return primitive(!p.value.double_);
+    case primitiveType::float_  : OCCA_FORCE_ERROR("Cannot apply operator ! to float type");   break;


These might be potentially used int he preprocessor, so we can't take them away

Ok, I can do something like return primitive(areBitwiseEqual(p.value.float_, static_cast<float>(0))?
Is it just this one, or do you want similar changes for && and ||? Since & and | are already disallowed in the current implementation I would say no?

Maybe for these operations, we can first cast the float/double as int

Ha ha, for float values, what is "zero" anyway?

My preference is the "...cannot apply operator ! to float type". Just leave it up to the user to explore the possibilities when a float type is involved based on his program logic. The user should already know he is in trouble if he's thinking about using logical not with a float value!

Allow me to illustrate.

For this primitive (not), is a sort of "near-!" acceptable, which checks for zero within macheps? e.g.,

case primitiveType::float_ : return primitive( ((float)1.0 + p.value.float_) == (float)1.0 )

My view: this is not acceptable for general use cases at this level. But others may disagree.

FYI possibly another approach to ! based on an equality to zero comparison without employing the explicit == (i.e., partial-order trick):

case primitiveType::float_ : return primitive( !(p.value.float_ < -p.value.float_ || -p.value.float_ < p.value.float_) )

All the above mutatis mutandis for the double case.

The argument made here is that we want to support as much C/C++ as possible, regardless of whether it's considered "good practice" or "bad practice". I 100% agree we shouldn't be doing exact comparisons with float or double "in practice". However, the goal here is language coverage.

TIL: Looks like floats aren't supported in the preprocessor!

main.cpp:4:6: error: floating constant in preprocessor expression #if !0.0

Since the preprocessor won't accept them anyway, can we keep these operators unimplemented for float/double? That would still be my preferred solution.

Sounds good!

awehrfritz · 2020-05-23T10:11:12Z

src/c/types.cpp

@@ -646,7 +651,7 @@ const int OCCA_PROPERTIES    = occa::c::typeType::properties;
 const occaType occaUndefined  = occa::c::undefinedOccaType();
 const occaType occaDefault    = occa::c::defaultOccaType();
 const occaType occaNull       = occa::c::nullOccaType();
-const occaUDim_t occaAllBytes = -1;
+const occaUDim_t occaAllBytes = static_cast<occaUDim_t>(-1);


Is this cast really necessary? I would hope the compiler can figure this out on its own, at least for such primitive types.

Clang reports it as an implicit conversion, so yes, it's needed to fix the warning.

OK. I just think this makes the code look ugly and cluttered. IMHO, code should be easy to read and intuitive, this is far from that. I would rather consider to add an exception (i.e. suppress the warning) for Clang than adding something like this to the code. Just my two cents...

How about we define a const value once and reuse it?

const occa::udim_t UDIM_DEFAULT = static_cast<occa::udim_t>(-1);

This would also make the code a bit clearer in places where -1 is set as the default because of how high the number is

@dmed256 that's probably a sensible compromise. I like the simple -1 since it's rather intuitive and easy to understand as is, but having a predefined constant works as well - anything that avoids cluttering the source code with type casts.

I've redone the relevant commit to use the const UDIM_DEFAULT, as suggested.

The testBadType call corrupts the data in the JSON object to be serialized. After this the roundtripping doesn't work anymore. Fix it by reordering the tests.

Floating point equality checks are complained about by many compilers. Changes to make them happy: * For C tests: change comparison to check for equality up to a small epsilon. Since we know we fill the arrays with integer numbers, this should suffice. * For C++ tests: check for bitwise equality of the floating point numbers. * For the general code: disallow various operations for floating point numbers that are wrapped in primitives. * For indexOf: add specialization for floating point that checks for bitwise equality. * Bitwise equality check for floating point numbers is tentatively placed in a new fp.hpp header.

dmed256 · 2020-05-31T02:02:17Z

Thank you to everyone that made this possible!

SFrijters force-pushed the fix-compiler-warnings branch from cd32c39 to 19eaa82 Compare April 22, 2020 09:52

dmed256 requested changes May 22, 2020

View reviewed changes

awehrfritz reviewed May 23, 2020

View reviewed changes

[Fix] Fix shadowing warnings

3e56817

SFrijters force-pushed the fix-compiler-warnings branch from 19eaa82 to 4b03c0b Compare May 29, 2020 12:30

SFrijters added 12 commits May 29, 2020 14:40

[Fix] Implicit conversions to unsigned integers

4073210

[Fix] Missing return statements for non-void functions

96411e5

[Test] Fix JSON serialization test

c1b2bf5

The testBadType call corrupts the data in the JSON object to be serialized. After this the roundtripping doesn't work anymore. Fix it by reordering the tests.

[Fix] Possibly unset variables

a82ceab

[Fix] Baseclass should be initialized explicitly in copy constructor

05a3a8a

[Fix] Implicit fallthrough

ca4b1f6

[Fix] Catching polymorphic type by value

efdefe0

[Fix] Add missing template specializations for areEqual

2f80d64

[Fix] Suppress unused variable warnings for OpenCL polyfill

86841df

[Fix] Conversion warnings by changing fabs to std::abs

05f7ac2

[CMake] Set warning flags for C code and enable -Werror for Debug builds

e144223

SFrijters force-pushed the fix-compiler-warnings branch from 4b03c0b to e144223 Compare May 29, 2020 12:41

Merge branch 'master' into fix-compiler-warnings

d60edee

dmed256 mentioned this pull request May 30, 2020

Enable use of CMake in github actions .yml #342

Closed

Merge branch 'master' into fix-compiler-warnings

9de1cf5

dmed256 merged commit 0523fa2 into libocca:master May 31, 2020

SFrijters deleted the fix-compiler-warnings branch June 8, 2022 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler warnings #329

Fix compiler warnings #329

SFrijters commented Apr 20, 2020 •

edited

Loading

codecov bot commented Apr 20, 2020 •

edited

Loading

noelchalmers commented Apr 21, 2020

SFrijters commented Apr 22, 2020

SFrijters commented Apr 22, 2020

dmed256 commented Apr 22, 2020

dmed256 left a comment

dmed256 May 22, 2020

SFrijters May 22, 2020 •

edited

Loading

dmed256 May 23, 2020

pdhahn May 23, 2020 •

edited

Loading

dmed256 May 23, 2020

SFrijters May 29, 2020

dmed256 May 31, 2020

awehrfritz May 23, 2020 •

edited

Loading

SFrijters May 23, 2020

awehrfritz May 23, 2020

dmed256 May 23, 2020

awehrfritz May 23, 2020 •

edited

Loading

SFrijters May 29, 2020

dmed256 commented May 31, 2020

Fix compiler warnings #329

Fix compiler warnings #329

Conversation

SFrijters commented Apr 20, 2020 • edited Loading

Description

codecov bot commented Apr 20, 2020 • edited Loading

Codecov Report

noelchalmers commented Apr 21, 2020

SFrijters commented Apr 22, 2020

SFrijters commented Apr 22, 2020

dmed256 commented Apr 22, 2020

dmed256 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SFrijters May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdhahn May 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awehrfritz May 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awehrfritz May 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmed256 commented May 31, 2020

SFrijters commented Apr 20, 2020 •

edited

Loading

codecov bot commented Apr 20, 2020 •

edited

Loading

SFrijters May 22, 2020 •

edited

Loading

pdhahn May 23, 2020 •

edited

Loading

awehrfritz May 23, 2020 •

edited

Loading

awehrfritz May 23, 2020 •

edited

Loading