[cfe-commits] [PATCH] [libcxx] cleanup a few compile warnings emitted by GCC

[Saleem Abdulrasool]

On Thu, Dec 27, 2012 at 10:49 AM, Howard Hinnant <hhinnant at apple.com> wrote:
> On Dec 25, 2012, at 11:54 AM, Saleem Abdulrasool <compnerd at compnerd.org> wrote:
>
>> This just rounds up a few compile warnings emitted by GCC (4.7.2).  Included
>> are:
>>    - missing field initializers
>>    - extraneous semicolon
>>    - unused variable
>>    - explicit instantiation of base class
>>    - comparisons between signed and unsigned values
>>    - array indexing via a char type
>>
>> http://llvm-reviews.chandlerc.com/D242
>>
>> Files:
>>  src/debug.cpp
>>  src/locale.cpp
>>  src/thread.cpp
>> <D242.1.patch>_______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> Partially accepted as noted below, committed revision 171165.  Thanks!
>
>> Index: src/debug.cpp
>> ===================================================================
>> --- src/debug.cpp
>> +++ src/debug.cpp
>> @@ -23,7 +23,7 @@
>>  {
>>      static __libcpp_db db;
>>      return &db;
>> -};
>> +}
>
> Ok, but src/debug.cpp is currently not working, and not being worked on.
>
>>  _LIBCPP_VISIBLE
>>  const __libcpp_db*
>> Index: src/locale.cpp
>> ===================================================================
>> --- src/locale.cpp
>> +++ src/locale.cpp
>> @@ -207,7 +207,8 @@
>>  }
>>
>>  locale::__imp::__imp(const __imp& other)
>> -    : facets_(max<size_t>(N, other.facets_.size())),
>> +    : facet(),
>> +      facets_(max<size_t>(N, other.facets_.size())),
>>        name_(other.name_)
>>  {
>>      facets_ = other.facets_;
>
> Rejected.  I prefer to not write code that the compiler will write for me.  If you need to insert a pragma to silence a warning, that would be my preferred way to deal with that.

That sounds fair.  Ive sent out another patch that adds the necessary
pragma for GCC.

>> @@ -4583,7 +4584,7 @@
>>  string
>>  __time_get_storage<char>::__analyze(char fmt, const ctype<char>& ct)
>>  {
>> -    tm t = {0};
>> +    tm t = {0,0,0,0,0,0,0,0,0,0,0};
>>      t.tm_sec = 59;
>>      t.tm_min = 55;
>>      t.tm_hour = 23;
>> @@ -4729,7 +4730,7 @@
>>  wstring
>>  __time_get_storage<wchar_t>::__analyze(char fmt, const ctype<wchar_t>& ct)
>>  {
>> -    tm t = {0};
>> +    tm t = {0,0,0,0,0,0,0,0,0,0,0};
>>      t.tm_sec = 59;
>>      t.tm_min = 55;
>>      t.tm_hour = 23;
>
> Rejected.  tm contains *at least* 9 int data members.  The portable and concise way to zero initialize this struct is with the single {0}.

Okay.  I agree that it can get unwieldy, and in light of that, I've
sent out another version of the patch that adds GCC specific pragmas
around the specific declarations (I tend to prefer to scope the
ignoring of warnings as tightly as possible ... but, I can understand
if you want to loosen it to the region where it is used.  I am more
than happy to update the patch to do that if you prefer).

>> @@ -4746,7 +4747,7 @@
>>      strftime_l(buf, 100, f, &t, __loc_);
>>      wchar_t wbuf[100];
>>      wchar_t* wbb = wbuf;
>> -    mbstate_t mb = {0};
>> +    mbstate_t mb = {0,{0}};
>>      const char* bb = buf;
>>  #ifdef _LIBCPP_LOCALE__L_EXTENSIONS
>>      size_t j = mbsrtowcs_l( wbb, &bb, sizeof(wbuf)/sizeof(wbuf[0]), &mb, __loc_);
>
> Rejected.  The internal structure of mbstate_t is not known.  There is no need for this code to assume an internal structure.  The portable and concise way to zero initialize this struct is with the single {0}.

Same as above.

> Index: src/thread.cpp
>> ===================================================================
>> --- src/thread.cpp
>> +++ src/thread.cpp
>> @@ -67,7 +67,7 @@
>>      return n;
>>  #elif defined(_POSIX_C_SOURCE) && (_POSIX_C_SOURCE >= 200112L) && defined(_SC_NPROCESSORS_ONLN)
>>      long result = sysconf(_SC_NPROCESSORS_ONLN);
>> -    if (result < 0 || result > UINT_MAX)
>> +    if (result < 0 || result > static_cast<long>(UINT_MAX))
>>          result = 0;
>>      return result;
>>  #else  // defined(CTL_HW) && defined(HW_NCPU)
>>
>
> Tentatively rejected.  This needs review by someone on this platform.  The code looks problematic to me when long and int are both 32 bit.

Actually, I double checked the return codes for sysconf, and it seems
that all errors are indicated by -1.  So, I changed this to an
explicit check for -1 as part of the patch that I sent out to cover
these additional issues.

Thanks!

> Howard
>

--
Saleem Abdulrasool
compnerd (at) compnerd (dot) org