Re: [lmi] [lmi-commits] master 98a3720 2/6: Explicitly qualify std::size_t

[Greg Chicares]

On 2/17/21 1:06 PM, Vadim Zeitlin wrote:
> On Wed, 17 Feb 2021 12:59:13 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> On 2/17/21 11:11 AM, Vadim Zeitlin wrote:
> [...]
> GC> > Should we
> GC> > (meaning I) add a coding rules check for this, i.e. verify that size_t 
> is
> GC> > always preceded by "std::" in all C++ files?
> GC> > 
> GC> >  Please let me know if you think this would be useful
> GC> 
> GC> No, I don't think so. Reasons, in order of increasing importance:
> GC> 
> GC> - It causes no immediate palpable harm, though of course a different
> GC> compiler, or a new version of the same compiler, may reject it.
> 
>  Yes, there is no real harm, it's "just" about consistency. But aren't
> many (most?) of the existing rules similar from this point of view?

Absolutely. In each case, I weigh perceived harm against the cost of
preventing it, and here I don't see much harm. Of course, the more I
look at something I dislike, the more harmful I perceive it to be,
and as the day progresses this seems harmfuller and harmfuller.

> GC> - False positives occur with almost any automated coding rule.
> 
>  This is true, but maybe I should have mentioned that I've actually tested
> that
> 
>       $ git grep '[^:s]size_t.' *.?pp

I had gotten this far:
  git grep '[^:]\<size_t\>'
but then I gave up.

That terminal '.' seems tricky--consider the consequence of adding
a name that lexicographically follows 'size_t':

-  #include <cstddef>  // size_t
+  #include <cstddef>  // size_t, something_else

but OTOH 'size_t' is the lexicographically last name in <cstddef>,
so it can't break unless they add something new to that header.

> finds no matches right now, so it looks like a check for this one would be
> pretty simple. This wouldn't detect (even not commented out) occurrences of
> size_t at the end of lines, but I don't think there is a real risk of this
> happening in practice.

Okay, committed and pushed: 24c32d6ed..8d4034b90  master -> master

For the record, I tested this alternative:

#if 0
    boost::sregex_iterator i(f.data().begin(), f.data().end(), r);
    boost::sregex_iterator const omega;
    for(; i != omega; ++i)
        {
        boost::smatch const& z(*i);
        if
            (f.leaf_name() != "test_coding_rules.cpp"
            )
            {
            std::ostringstream oss;
            oss
                << "contains unqualified 'size_t': '"
                << z[0]
                << "'."
                ;
            complain(f, oss.str());
            }
        }
#endif // 0

but rejected it:
 - it's slower prima facie
 - it gives very little more information
 - we'll seldom be writing 'size_t' anyway

> GC> - Isn't it virtually always better to use 'int' instead of 'size_t'
> GC> these days? If so, then it's virtually always a mistake to write
> GC> 'size_t' at all. We've mostly replaced it with 'int' already, so
> GC> the remaining occurrences are a code smell. For example, this
> GC> declaration looks like a mistake:
> GC> 
> GC> // ABI:
> GC> extern "C" char* __cxa_demangle
> GC>     (char const * mangled_name  // mangled name, NUL-terminated
> GC>     ,char       * output_buffer // just use 0
> GC>     ,std::size_t* length        // just use 0
> GC>     ,int        * status        // zero --> success
> GC>     );
> GC> 
> GC> If we need a prototype for that, we should include some header.
> GC> But AFAICS gcc doesn't require a prototype for __cxa_demangle
> GC> because it's built in. I think I wrote that only as documentation,
> GC> so it should be in a comment.
> 
>  Err, gcc definitely requires __cxa_demangle to have this prototype and on
> the platforms where size_t and int have different size (such as Win64), it
> would be a fatal mistake to use int here. Similarly, we still need to use
> size_t when overriding base class virtual functions using it.

The change I propose is not

-     ,std::size_t* length        // just use 0
+     ,int*         length        // just use 0

but rather

-// ABI:
+// For reference, the ABI specifies this prototype:
+// extern "C" char* __cxa_demangle
+//     (char const * mangled_name  // mangled name, NUL-terminated
+//     ,char       * output_buffer // just use 0
+//     ,std::size_t* length        // just use 0
+//     ,int        * status        // zero --> success
+//     );

But the change is a little larger than that, and rather than
type it all here, I'll just commit it (master 064c03382).

We could do this:
+ #include <cxxabi.h>
but that's already done implicitly, if I understand this correctly:

  https://gcc.gnu.org/onlinedocs/libstdc++/libstdc++-html-USERS-4.3/a01845.html
00033 /* This file declares the new abi entry points into the runtime. It is not
00034    normally necessary for user programs to include this header, or use the
00035    entry points directly. However, this header is available should that be
00036    needed.

However, upon reexamination, I see that <cxxabi.h> already
is #included wherever it would even potentially make sense:

$git --no-pager grep -L '<cxxabi.h>' `git grep -l __cxa` 
test_coding_rules.cpp

so the point is moot.