New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix all errors and warnings in CMake, VS2017RC and Xcode. #156
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
@@ -340,7 +340,7 @@ static void MaybeAppendWithLength(State *state, const char * const str, | |||
// A convenient wrapper arount MaybeAppendWithLength(). | |||
static bool MaybeAppend(State *state, const char * const str) { | |||
if (state->append) { | |||
int length = StrLen(str); | |||
auto length = StrLen(str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This introduces an unconditional dependency to C++11. However, glog is a C++98/03 library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. Is it ok now?
CLAs look good, thanks! |
Changed from auto to size_t. |
On a fresh checkout on Windows a Cmake run does not finish because cmake can't find glags-config.cmake and a check for pthread fails. With both set to OFF it works flawlessly.
Now all errors and warnings from CMake, Visual Studio 2017 RC and Xcode 8.2.1 are fixed. Please pull. |
option (WITH_GFLAGS "Use gflags" ON) | ||
option (WITH_THREADS "Enable multithreading support" ON) | ||
option (WITH_GFLAGS "Use gflags" OFF) | ||
option (WITH_THREADS "Enable multithreading support" OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not change the defaults. Gflags and Threads are both optional features, even if the WITH_*
options are set to ON
. The messages you see during generation are not errors, but warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you a suggestion for a error- and warning-free default checkout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simply ignore the warnings about missing gflags-config.cmake
. There are also CMake means for disabling packages using the command-line. You'll have to check the CMake documentation.
Are there any other warnings/errors that prevent generation? A log output would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You made some doubtful changes. Please see my comments. Also, it would make sense either to squash the commits into a single one, or consider submitting several pull requests.
@@ -191,7 +191,7 @@ GLOG_DEFINE_string(log_backtrace_at, "", | |||
#include <BaseTsd.h> | |||
#define ssize_t SSIZE_T | |||
#endif | |||
static ssize_t pread(int fd, void* buf, size_t count, off_t offset) { | |||
static ssize_t pread(int fd, void* buf, unsigned int count, off_t offset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes from size_t
to unsigned int
do not look right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I changed it to size_t again and added a cast for read which uses unsigned int.
@@ -1436,7 +1436,7 @@ void LogMessage::SendToLog() EXCLUSIVE_LOCKS_REQUIRED(log_mutex) { | |||
LogDestination::WaitForSinks(data_); | |||
|
|||
const char* message = "*** Check failure stack trace: ***\n"; | |||
if (write(STDERR_FILENO, message, strlen(message)) < 0) { | |||
if (write(STDERR_FILENO, message, (unsigned int)strlen(message)) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change the parameter type if you still need to perform a type cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to #pragma warning( disable: 4267 ) since on Windows strlen returns size_t while write uses unsigned int.
@@ -2019,6 +2019,9 @@ LogMessageFatal::LogMessageFatal(const char* file, int line, | |||
const CheckOpString& result) : | |||
LogMessage(file, line, result) {} | |||
|
|||
#ifdef _MSC_VER | |||
# pragma warning(disable: 4722) | |||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use push/pop to restore the warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
On Windows strlen return size_t while _write uses unsigned int.
I addressed all comments except the one about disabling WITH_ defaults. How can I make this warning go away? |
@gonsolo See my previous comment. The warning (which is of informative nature) can be simply ignored. |
This introduces warnings again.
Fixed whitespace and rebased. Should be ready to pull now.
…On Wed, Jan 18, 2017 at 2:13 PM, Gonsolo ***@***.***> wrote:
Found a even better solution: The typedef is a compile check so it is just
marked unused.
On Wed, Jan 18, 2017 at 1:27 PM, Gonsolo ***@***.***> wrote:
> Ok, found it. The typedef is defined in a macro and subsequently not
> used. Removed it and pushed.
>
> On Wed, Jan 18, 2017 at 12:52 PM, Sergiu Deitsch <
> ***@***.***> wrote:
>
>> ***@***.**** commented on this pull request.
>> ------------------------------
>>
>> In src/logging_unittest.cc <#156>:
>>
>> > @@ -251,8 +251,8 @@ void TestLogging(bool check_counts) {
>> PLOG_EVERY_N(ERROR, 2) << "Plog every 2, iteration " << COUNTER;
>> errno = old_errno;
>>
>> - LOG_EVERY_N(ERROR, 3) << "Log every 3, iteration " << COUNTER << endl;
>> - LOG_EVERY_N(ERROR, 4) << "Log every 4, iteration " << COUNTER << endl;
>> + //LOG_EVERY_N(ERROR, 3) << "Log every 3, iteration " << COUNTER << endl;
>> + //LOG_EVERY_N(ERROR, 4) << "Log every 4, iteration " << COUNTER << endl;
>>
>> Maybe using __attribute__((unused)) can help? The typedef is likely
>> generated using macros. Try searching for a small part of typedef's name.
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <#156>, or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/AB8npAm5eGxxZNjXaJ4G9cGcJ48nCsP3ks5rTf0ZgaJpZM4LkqOe>
>> .
>>
>
>
>
> --
> g
>
--
g
--
g
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some problems left, unless you forgot to commit some of your changes.
@@ -932,8 +932,14 @@ struct CrashReason; | |||
bool IsFailureSignalHandlerInstalled(); | |||
} // namespace glog_internal_namespace_ | |||
|
|||
#ifdef __APPLE__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not Apple specific. GCC and Clang support the attribute.
@@ -795,7 +795,7 @@ static void TestOneTruncate(const char *path, int64 limit, int64 keep, | |||
const size_t buf_size = statbuf.st_size + 1; | |||
char* buf = new char[buf_size]; | |||
memset(buf, 0, buf_size); | |||
CHECK_ERR(read(fd, buf, buf_size)); | |||
CHECK_ERR(read(fd, buf, (unsigned int)buf_size)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you cast here?
@@ -786,7 +794,7 @@ inline void LogDestination::LogToSinks(LogSeverity severity, | |||
size_t message_len) { | |||
ReaderMutexLock l(&sink_mutex_); | |||
if (sinks_) { | |||
for (int i = sinks_->size() - 1; i >= 0; i--) { | |||
for (ptrdiff_t i = sinks_->size() - 1; i >= 0; i--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thought about this. You can write
for (size_t i = sinks_->size() - 1; i < sinks_->size(); --i)
// ...
If i
overflows, the loop will end. This is also safe, since unsigned integers are guaranteed to wrap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Is ptrdiff_t ok though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need ptrdiff_t
if you use the loop above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure I will get a warning about truncation from size_t (sinks_->size()) to int (i). This was the reason to change the loop in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't be right. sinks_
is of type std::vector<LogSink*>
whose size
method returns a type compatible to size_t
. So the proposed should be just fine. I mean
for (size_t i = sinks_->size() - 1; i < sinks_->size(); --i)
will work here without casts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you can use std::vector<LogSink*>::reverse_iterator
.
@@ -796,7 +804,7 @@ inline void LogDestination::LogToSinks(LogSeverity severity, | |||
inline void LogDestination::WaitForSinks(LogMessage::LogMessageData* data) { | |||
ReaderMutexLock l(&sink_mutex_); | |||
if (sinks_) { | |||
for (int i = sinks_->size() - 1; i >= 0; i--) { | |||
for (ptrdiff_t i = sinks_->size() - 1; i >= 0; i--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before.
@@ -628,7 +628,7 @@ inline void LogDestination::RemoveLogSink(LogSink *destination) { | |||
MutexLock l(&sink_mutex_); | |||
// This doesn't keep the sinks in order, but who cares? | |||
if (sinks_) { | |||
for (int i = sinks_->size() - 1; i >= 0; i--) { | |||
for (ptrdiff_t i = sinks_->size() - 1; i >= 0; i--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my next comment.
off_t orig_offset = lseek(fd, 0, SEEK_CUR); | ||
if (orig_offset == (off_t)-1) | ||
return -1; | ||
if (lseek(fd, offset, SEEK_CUR) == (off_t)-1) | ||
return -1; | ||
ssize_t len = write(fd, buf, count); | ||
ssize_t len = write(fd, buf, (unsigned int)count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should avoid the cast here.
@@ -207,7 +207,7 @@ static ssize_t pread(int fd, void* buf, size_t count, off_t offset) { | |||
#endif // !HAVE_PREAD | |||
|
|||
#ifndef HAVE_PWRITE | |||
static ssize_t pwrite(int fd, void* buf, size_t count, off_t offset) { | |||
static ssize_t pwrite(int fd, void* buf, unsigned int count, off_t offset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change the parameter type again?
off_t orig_offset = lseek(fd, 0, SEEK_CUR); | ||
if (orig_offset == (off_t)-1) | ||
return -1; | ||
if (lseek(fd, offset, SEEK_CUR) == (off_t)-1) | ||
return -1; | ||
ssize_t len = read(fd, buf, count); | ||
ssize_t len = read(fd, buf, (unsigned int)count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the type cast?
The source problem is that strlen returns size_t but _read and _write use int on Windows. So somewhere it has to be cast. I tried to leave the source mostly as it was now (or changed to size_t) and just disabled the warnings where it happened. |
Fixed and rebased again. Next try. :) |
This can be right. sinks_ is of type std::vector<LogSink*> whose size
method returns a type compatible to size_t. So the proposed should be
just fine.
I was talking of the original version where sinks_ returned size_t but it
was assigned to an int.
…--
g
|
Or you can use std::vector<LogSink*>::reverse_iterator.
Can we just stick with ptrdiff_t? I just wanted to get rid of the warning
and not think of the program logic too much. Changing this could introduce
different semantics or subtle errors.
…--
g
|
FWIW, I'm using glog in a bigger project and I just tried to get rid
of all warnings so new warnings in the project will stand out clearly.
…--
g
|
@gonsolo The way the loop is implemented now is incorrect. By using my proposal you will fix this and get rid of the corresponding warnings. |
@gonsolo <https://github.com/gonsolo> The way the loop is implemented
now, is incorrect. By using my proposal you will fix this and get rid of
the corresponding warnings.
Old version:
for (int i = sinks_->size() - 1; i >= 0; i--) {
My version:
for (ptrdiff_t i = sinks_->size() - 1; i >= 0; i--) {
I don't get it, what's wrong here?
…--
g
|
@gonsolo Correct me if I'm wrong, but |
@gonsolo <https://github.com/gonsolo> Correct me if I'm wrong, but size()
return value (which is unsigned) is truncated by conversion to ptrdiff_t
(which is signed).
Yes. The old version truncated to 32 bits signed (int) and the new version
to 64 bit signed (signed). VS warns about the former but not the latter. So
it's progress. :)
Anyway, I changed it to reverse iterators.
There is one pop_back in a loop but I guess it's ok since it only
invalidates the last element, right?
…--
g
|
Anything else I can improve? |
@gonsolo Could you please resolve the conflicts? |
No, it's been nearly three years and I stopped using glog. |
This fixes a warning about truncation from size_t (64 bits on 64 bit Windows) to int (32 bits on 64 bit Windows).