Azure / azure-storage-cpp

Microsoft Azure Storage Client Library for C++
http://azure.github.io/azure-storage-cpp
Apache License 2.0
132 stars 147 forks source link

Fix problem with sanitize options on modern versions of gcc #313

Closed ghalliday closed 4 years ago

ghalliday commented 4 years ago

Some of the constructors for request_result do not initialize the value leading to complaints when the copy constructor is used.

Signed-off-by: Gavin Halliday gavin.halliday@lexisnexis.com

ghalliday commented 4 years ago

I could not find a valid link to the submission agreement, please can you supply one. Although I have filled in one for other open source projects - possibly why check came back passed.

I may submit other fixes relating to more modern gcc compilers if all goes to plan.

Jinming-Hu commented 4 years ago

Hi @ghalliday , thanks for your contribution.

leading to complaints when the copy constructor is used.

But I don't understand why compiler would give a warning on copy constructor.

As far as I can see, the compiler should give warnings on the two constructors in this file request_result.cpp, since m_request_server_encrypted is not always initialized here.

I think your fix of using in-class member initializer is great. The problem is, this project started long time ago, when C++ 11 was not so widely popularized as today. So in-class member initializer would not be consistent with existing initialization list.

So I think it's a better idea either we initialize m_request_server_encrypted in every constructor, or we initialize other variables m_is_response_available, m_target_location, m_http_status_code, m_content_length and m_request_server_encrypted also with in-member class initializer.

ghalliday commented 4 years ago

The sanitize checks in gcc 9/clang are performed at runtime, rather than compile time, and are looking for undefined behaviour.

I think it gives an error on the copy constructor because that copies the uninitialized field from the source to the target, and the source value is not a valid boolean value - because it has not been initialized.

There are also various warnings that come out of modern compilers which would also probably catch these uninitialized members, and other errors. I have ignored them for the time being to get my initial code working.

The project I work on has a similar issue - it started well before c++11 existed. We have decided to gradually move towards using initializers on members because it avoids issues like this.

What is your preferred solution? Ensure they are initialized in all constructors, or use initializers on the members?

Jinming-Hu commented 4 years ago

Hi @ghalliday,

For a project starting from scratch, I prefer in-class initializer.

But considering that initialization lists are used everywhere in this project, I'm inclined to think initializing m_request_server_encrypted here is a better idea, which also takes least effort. (I suppose this can also get rid of all the errors and warnings you mentioned.)

ghalliday commented 4 years ago

ok. I will take a look at reimplementing the change.

ghalliday commented 4 years ago

@JinmingHu-MSFT updated