Handle all MSVC level 4 warnings (issue #19) #34

MathiasMagnus · 2017-04-12T07:13:04Z

Although there was explicit statement that we wanted to eliminate all warnings, I believe it would benefit all, if we could develop new features on the highest warning level of all compilers and receive messages only related to the feature being implemented.

As it stands, it is by far a non-zero amount of work, so I wanted to break down this "feature" implementation into to parts, in case there would be no consensus this is desired feature. Most of the warnings were not actual errors in the code, but all of them are useful to identify potential warnings.

Some notes

The PR surely needs additional commits, as the code isn't clean. In most cases I just wanted to mark code that triggers warnings and silence them.

warning C4456: declaration of '' hides previous local declaration

This is fairly straightforward. Triggers when one overrides a scoped variable name locally, for instance int i with the running index of a for loop. This mostly triggered due to template code overriding variables in the cpp files. Not an error, as the template code surely did not wish to reference the cpp version of the variables.

warning C4127: conditional expression is constant

Probably the most noisy change. It is supposed to trigger on cases such as if(true) and if(false). It also triggers however on template code, in cases where the boolean is a constexpr value. To silence the warning, I changed it to proper template code. (Although I recall not trying assigning the value to a runtime variable.) Should be vastly cleaner one if constexpr() will be widely supported.

warning C4459: declaration of '' hides global declaration

Similar to the local case, this triggers when template code created variables that are global variables in the cpp and they are hidden. I made those variables local in the cpp, as in most cases they were not needed to be global variables. (Bad practice in my view. I try avoiding global variables whenever possible.)

warning C4100: '': unreferenced formal parameter

Triggers when function parameters are given names, but then are not used. I mostly commented out their names, as they occured in unimplemented functions, but I wanted to leave a mark of their nature. (What does a string_type param refer to? Id? extension name?) When it was the last param and was given a default value, I couldn't comment it out as it changes the overload set and breaks code. In these cases, I mentioned them in the functions body and noted.

Most notably in the case of allocators it was strange we never use them.

Onward

If you think this is a worthwhile effort, I will go on with Warning Level 3 messages. Once I finish, I believe there will be much less work for fixing all issues to have GCC and Clang compile warning free as well. Most noisy of all on Ubuntu 16.04 with stock OpenCL headers is

warning: ignoring attributes on template argument

Triggers when the template machinery strips GCC specific attributes from template parameters (just about EVERY template function inside Boost Compute).

keryell

Great amount of work as usual!

While I agree with most of the changes, there are a few things that look more dubious to me.

Most notably in the case of allocators it was strange we never use them.

Yes there is a lot of stuff to be implemented... :-/

Currently there are a lot of warning with Boost.Compute/OpenCL/GCC on Ubuntu 16.10. Let's wait for 17.04 to see if there are some improvements before digging into this.

Thank you for your contribution.

keryell · 2017-04-12T13:16:59Z

include/CL/sycl/buffer/detail/buffer.hpp

+  /** Template specialization implementing the no op branch
+  */
+  template <>
+  void track_access_mode<access::mode::read>() {}


Quite clearer. :-)

keryell · 2017-04-12T13:20:31Z

include/CL/sycl/pipe/detail/pipe_accessor.hpp

+  template <> void dtor_dispatch<access::mode::write>() {
+    implementation->used_for_writing = false;
+  }
+


I guess that when we switch to C++17 we can use if constexpr() some day...

keryell · 2017-04-12T13:24:06Z

tests/buffer/associative_containers.cpp

@@ -10,7 +10,7 @@

 using namespace cl::sycl;

-int test_main(int argc, char *argv[]) {
+int test_main(int /*argc*/, char** /**argv[]*/) {


I find the previous type of arg clearer, since it correctly suggest it is an array of C-style strings.
Every time I use a pointer in my C++ programs, it is a kind of personal failure. So if I can minimize the number of * I am happier. :-)

Is there a strong compelling argument for these changes?

No, nothing strong. I just never wrote plain char*[] as a type before and the linker was arguing about char** missing as a signature type of test_main, so I just went along with what the linker suggested. What's the harm if it's unused anyway?

Anyhow, changed all occurences with char*[] /*argv*/.

keryell · 2017-04-12T13:27:18Z

tests/buffer/global_buffer_set_final_data.cpp

+int test_main(int /*argc*/, char** /**argv[]*/) {
+  constexpr size_t N = 16;
+
+  buffer<int> buf{ N };


As you can see in the test name and in the comments you removed, this is for testing a global buffer case.

If you make the buffer an automatic variable, well, it is another test...

Apologies, it was my understanding "global" refers to the global memory namespace the buffer resides in. It does make more sense this way however. :)

I will rename the variable to something more unique, because somewhere inside the headers, there is a local variable named buf and that's what triggers the warning. Most likely I will rename the header version to something more unique and leave nice names like b and buf for user code.

keryell · 2017-04-12T13:29:23Z

tests/jacobi/include/helpers-jacobi.hpp

@@ -77,7 +77,7 @@ int es = PAPI_NULL; //event set

 size_t NB_ITER = CONST_NB_ITER;
 size_t M = WG_MULT0*J_CL_DEVICE_CONST_WORK_GROUP_SIZE0+JACOBI_DELTA;
-size_t N = WG_MULT1*J_CL_DEVICE_CONST_WORK_GROUP_SIZE1+JACOBI_DELTA;
+size_t K = WG_MULT1*J_CL_DEVICE_CONST_WORK_GROUP_SIZE1+JACOBI_DELTA;


Curious this change...

Is this an automatic Visual Studio feature? :-)

In the following, in the same way we have (i, j), it is common to have (M, N) for the size instead of (M, K).
Perhaps I touched too much Fortran 77 in the past... :-)

The same in all the Jacobi tests.

Again, the global variable named N conflicts with some template code which will hide the global instance of the id. (I believe buffer dimensions are also referred to with N) I used the "Rename" feature of VS here, which is a bit smarter than Find and Replace all, as it only changes the references to the instance in question, and not places where the name is hidden. (This is the perfect example when Rename is smarter than FnR.)

On a more general basis, shouldn't we prefix library identifiers with something to separate them from user code a bit better? Prefixing with a simple underscore is reserved for CRT and STL implementations (for this very reason), but something uniform and unique could be made coherent across the headers.

MathiasMagnus · 2017-04-13T13:03:08Z

I changed the test_main signatures to have less asterisks, also moved some of the changes into the library from user code (aka. unit tests), such as the global buffer test case with id "buf" and also the Jacobian tests using "N" as ids. Changed the conflicting library part to have a more unique internal id name.

MathiasMagnus · 2017-04-15T13:32:02Z

While I was at it, I went ahead and fixed all other warnings.

Good news

ALL triSYCL unit tests compile warning free on practically the highest warning level! The only unit test that does not pass is a sync issue already referenced under issues.

Bad news

The Jacobian tests have a significant amount of type casting, due to the code sticking much to using ints as indexes, however all API functions return size_ts for index values. Because it was stated that the Jacobian tests will receive an overhaul, I did not intend on making the changes pretty. When somebody comes to remove code duplication and clean it up, they can handle most of it. My changes are not ugly, I just aimed at making the compiler happy.

One warning could not be disabled, because it originates from Boost, the multi-array library to be more precise. Boost multi-array uses std::copy under the hood, but does not use checked iterators, which is a feature of the MSVC Debug Iterators, which when used only accepts checked iterators when using raw pointers. In summary, when one wants to feed raw pointers to STL algorithms, the debug iterator machinery would mandate to wrap it into a different type (still essentially a raw iterator + size) to silence the potentially unsafe raw pointer warning. By wrapping it into a different type (and also specifying size), the programmer makes an explicit promise (that is checked in debug builds), that there will be no overindexing with the provided raw pointers.

Because Multi-array does not use this feature, I could not turn it off. If I'll come up with a good and safe way to do so, (pragma warning push/pop) I'll do so in a later PR.

Notes

The most common issue was assuming unsigned int and size_t are identical types. Some more serious misconceptions were notoriously assigning size_ts to ints and vice versa. Some double-float casting issues, as well as implicit casts from integral to floating types, which the compiler detects as possible loss of data.

keryell · 2017-04-24T07:58:43Z

Hi!
Unfortunately I am drown by various deadlines for now, such as the Khronos face-to-face meeting in Amsterdam starting now, so I do not have the bandwidth to look at it. :-(
Be patient. You can always put some other pull-request in the pipeline in the meantime...:-)
Thanks.

MathiasMagnus · 2017-04-24T18:23:05Z

No worries. Similar issues here. I already found that my changes broke the build with Clang because of an extension I use (specializing member classes in class scope, which is a totally legit thing to do, just not part of the core language unfortunately). I'll have to go patch those up before merging anyway.

keryell · 2017-05-05T09:44:59Z

It does not compile for me:

/home/keryell/Xilinx/Projects/OpenCL/SYCL/triSYCL/branch/tmp/include/CL/sycl/pipe_reservation/detail/pipe_reservation.hpp: At global scope:
/home/keryell/Xilinx/Projects/OpenCL/SYCL/triSYCL/branch/tmp/include/CL/sycl/pipe_reservation/detail/pipe_reservation.hpp:79:13: error: explicit specialization in non-namespace scope ‘class cl::sycl::detail::pipe_reservation<PipeAccessor>’
   template <> void ctor_dispatch<access::mode::write>(std::size_t s) {

But honestly do not spoil your precious time on do all this template specialization.
It makes the code less readable and we will switch to C++17 later this year anyway with "if constexpr()"...

But I guess than even now, through inlining and partial evaluation, the compiler should already generate a specialized code...

But anyway, there are still a lot to be implemented before diving into these optimizations. :-)

MathiasMagnus · 2017-05-11T14:02:11Z

MSVC support constexpr if in VS2017 Preview 2 using /std:c++17 and /std:c++latest as well as /std:c++14 with an added warning, that this is actually a C++17 feature. (This is so the STL may use it internally for faster compiles in C++14 mode. I am sure the Clang folk will be delighted there's a feature breach inside the STL they will have to handle when building on Windows.)

I'll go ahead and revert the changes, perhaps with an ifdef or some more elaborate CMake configure check. Luckily GCC and Clang don't spew the warning in question, do they are not affected.

keryell · 2017-05-12T14:28:54Z

Nice to see Visual Studio making some progress!
But it is not clear what is the action to do? Switching triSYCL to C++17 or at least C++1z?
Since Clang 4.0 is necessary to have all tests passing, it might not be an issue.
But constexpr ifare only in GCC 7 https://gcc.gnu.org/projects/cxx-status.html and the final release 7.1 is pretty recent (https://gcc.gnu.org/ : 2017/05/02), which is not in Debian/unstable today or Ubuntu 17.04.
So if we say that triSYCL should work on any standard recent distribution with common compilers, that would mean for example Ubuntu 17.10. So switching to C++17 on October 2017. :-)
In the meantime standard if are just fine, if we consider all what is to be implemented before the micro-optimizations...

MathiasMagnus · 2017-06-06T08:24:15Z

I do revisit the issue from time to time, recently with the firm intention of pushing the PR through by reverting the static dispatch parts, re-simplifying the code and silencing the warning C4127: conditional expression is constant messages by exploiting the very same language feature breach that MSVCs STL does (will do) by allowing if constexpr to be used while issuing a warning of non-conforming code. This is the solution with minimal impact on the code. Something like:

if TRISYCL_CONSTEXPR_IF (  Mode != access::mode::read )
{ ... }

However, I keep coming back to the same issue: Boost. (I detest, despise, hate, etc. the collective of Boost libraries. They are borderline impossible to depend upon in a portable manner. In all my code I try to stay as far away from them as possible.)

Boost.Build is a crime against humanity. Nobody outside the Boost community uses it, and for good reason.
Boost is the playground for cutting-edge C++, yet it fails to compile with cutting-edge compilers. A project of such scope, I'd expect that conformance tests are ran against pre-release SW, but no. Boost philosophy is: do not develop against pre-release compilers. This was the official statement when VS15 was already in Release Candidate state before 1.64 feature freeze. Heck, it's past Beta, and the very purpose of a Beta is to start testing against it. RC is a promise of minimal changes till release. Hence, 1.64 was released not supporting it. A recent release has v141 toolset support, but I fail to compile any code using it. There is a hardcoded configure mishap. C:\Kellekek\Boost\1.64.0\boost\typeof\msvc\typeof_impl.hpp(125): error C2988: unrecognizable template declaration/definition and the line in question: template<> struct id2type_impl<true> //VC8.0 specific bugfeature. That is the kind of comment I like to see in active pre-processor regions. 16 year old workarounds being active.
Yes, did I say all compiler feature capability tests are hardcoded? No wonder Boost libraries barely compile. ALL compilers require explicit configure script support and manual testing. One might think they'd ditch Boost.Build in favor of... CMake? No. 2 feature complete Cmake-ifying attempts were killed off and were not mainlined.
Don't even get me started on compiling Boost.MPI with MS-MPI... dive deep into configure.jam scripts. Yummy!

Bottom line is, for various reasons, triSYCL never compiles with MSVC. As MSVC conformance progresses, Boost has out-of-date workarounds that trigger for no reason (hardcoded configure headers), I either solve the issue of keeping old compilers around and not using new C++ features or simply not use Boost. I generally do the latter, but triSYCL heavily depends on Boost. Maybe Boost 1.65, a few months from now maybe they'll have proper v141 (or v142) toolset support, at least with /std:c++14. /std:c++latest will never work with the current design of Boost.Build.

keryell · 2017-06-12T14:44:57Z

I do not have any opinion about Boost.Build as I do not write or package some Boost libraries. :-)

But globally Boost is quite useful for a lot of people and specially for me here so I cannot imagine moving away from it.

It looks like there have been/are some efforts on moving Boost towards CMake:
https://github.com/boost-cmake
https://github.com/boost-cmake/bcm

I will ask some people at the various C++ Meetup and committees to have some feedback about all of this.

That said, as a Windows user, you are a user of a professional operating system and you pay for this. So you should also buy ComputeCpp for Windows to have something working and supported instead of using open source software such as Linux, Boost or triSYCL that worth nothing since they are free. ;-)

keryell · 2018-11-27T02:40:41Z

Any change with the latest MSVC, triSYCL & Boost.Compute?

keryell · 2020-04-23T01:47:18Z

While I was at it, I went ahead and fixed all other warnings.

Bad news

The Jacobian tests have a significant amount of type casting, due to the code sticking much to using ints as indexes, however all API functions return size_ts for index values. Because it was stated that the Jacobian tests will receive an overhaul, I did not intend on making the changes pretty. When somebody comes to remove code duplication and clean it up, they can handle most of it. My changes are not ugly, I just aimed at making the compiler happy.

Recently I had to fix a few warnings in this old code. This should be completely rewritten and right now it cannot run but on the host device... :-(

MathiasMagnus added 7 commits March 7, 2017 14:57

CL/sycl/buffer/detail/buffer.hpp(65): error C3855

2e771d9

warning C4456: declaration of '<id>' hides prev

2143787

warning C4127: conditional expression is constant

cf0de74

warning C4456 + C4127 + VS Open Folder support

1ef9308

few more fixes

2bdfebe

warning C4459 fixed.

08b04d3

warning C4100: '<param>': unreferenced formal parameter

5a82c77

keryell self-assigned this Apr 12, 2017

keryell requested changes Apr 12, 2017

View reviewed changes

Modification upon request.

995f698

MathiasMagnus added 6 commits April 13, 2017 21:44

fixed warning C4189

a396023

fixed warning C4700

083f571

fixed warning C4101

6f09673

fixed warning C4305

5b50649

fixed warning C4244

cbcf629

fixed warning C4267

fa81de5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle all MSVC level 4 warnings (issue #19) #34

Handle all MSVC level 4 warnings (issue #19) #34

MathiasMagnus commented Apr 12, 2017

keryell left a comment

keryell Apr 12, 2017

keryell Apr 12, 2017

keryell Apr 12, 2017

MathiasMagnus Apr 13, 2017

keryell Apr 12, 2017

MathiasMagnus Apr 13, 2017

keryell Apr 12, 2017

MathiasMagnus Apr 13, 2017

MathiasMagnus Apr 13, 2017

MathiasMagnus commented Apr 13, 2017

MathiasMagnus commented Apr 15, 2017

keryell commented Apr 24, 2017

MathiasMagnus commented Apr 24, 2017

keryell commented May 5, 2017

MathiasMagnus commented May 11, 2017 •

edited

keryell commented May 12, 2017

MathiasMagnus commented Jun 6, 2017

keryell commented Jun 12, 2017

keryell commented Nov 27, 2018

keryell commented Apr 23, 2020 •

edited

Bad news

Handle all MSVC level 4 warnings (issue #19) #34

Are you sure you want to change the base?

Handle all MSVC level 4 warnings (issue #19) #34

Conversation

MathiasMagnus commented Apr 12, 2017

Some notes

warning C4456: declaration of '' hides previous local declaration

warning C4127: conditional expression is constant

warning C4459: declaration of '' hides global declaration

warning C4100: '': unreferenced formal parameter

Onward

warning: ignoring attributes on template argument

keryell left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathiasMagnus commented Apr 13, 2017

MathiasMagnus commented Apr 15, 2017

Good news

Bad news

Notes

keryell commented Apr 24, 2017

MathiasMagnus commented Apr 24, 2017

keryell commented May 5, 2017

MathiasMagnus commented May 11, 2017 • edited

keryell commented May 12, 2017

MathiasMagnus commented Jun 6, 2017

keryell commented Jun 12, 2017

keryell commented Nov 27, 2018

keryell commented Apr 23, 2020 • edited

Bad news

MathiasMagnus commented May 11, 2017 •

edited

keryell commented Apr 23, 2020 •

edited