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

[Howard Hinnant]

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.

> @@ -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}.

> @@ -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}.

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.

Howard