GDB 7.99.91 MinGW compilation error in cli-script.c

  > Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Tue, 23 May 2017 10:53:47 +0100
> 
> >> It'd be useful for the archives if you expanded on which mingw versions
> >> and compilers you tested this on.  Also likewise a short comment to
> >> the effect in the code would be likewise handy for future readers
> > 
> > OK, will do.

Here's the final version; okay to commit?
  

> Here's the final version; okay to commit?
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index a96e71f..d3a251b 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2017-05-24  Eli Zaretskii  <eliz@gnu.org>
> +
> +	* common/common-utils.h (REPLACE_TO_STRING) [__MINGW32__]: Define
> +	to 1 if std::to_string is not available.
> +	(std::to_string) [REPLACE_TO_STRING]: Provide a replacement
> +	implementation.

Did we consider the option of perhaps only pushing this patch
to the 8.0 branch, and require MinGW 5.x for the current master?
If we did it that way, it would allow us to avoid remembering
that we need to use gdb::to_string instead of std::to_string.
  

On 05/24/2017 08:37 PM, Joel Brobecker wrote:

>> +2017-05-24  Eli Zaretskii  <eliz@gnu.org>
>> +
>> +	* common/common-utils.h (REPLACE_TO_STRING) [__MINGW32__]: Define
>> +	to 1 if std::to_string is not available.

This:

>> +	(std::to_string) [REPLACE_TO_STRING]: Provide a replacement
>> +	implementation.

Should really be:

	(gdb::to_string): Define.

and you need an entry for the cli/cli-script.c change, like:

        * cli/cli-script.c (user_args::insert_args): Use it.

Otherwise this version of the patch is OK with me.

> Did we consider the option of perhaps only pushing this patch
> to the 8.0 branch, and require MinGW 5.x for the current master?
> If we did it that way, it would allow us to avoid remembering
> that we need to use gdb::to_string instead of std::to_string.

That'd be fine with me, FWIW.

Thanks,
Pedro Alves
  

> Cc: simon.marchi@polymtl.ca, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 25 May 2017 11:12:22 +0100
> 
> This:
> 
> >> +	(std::to_string) [REPLACE_TO_STRING]: Provide a replacement
> >> +	implementation.
> 
> Should really be:
> 
> 	(gdb::to_string): Define.

The code says std::to_string, though.  So it sounds like some coding
conventions are being applied here of which I wasn't aware, and
neither is Emacs.  Are these conventions described somewhere?

> and you need an entry for the cli/cli-script.c change, like:
> 
>         * cli/cli-script.c (user_args::insert_args): Use it.

I added that, but once again, the convention to put the
fully-qualified symbol name in the log entry should be documented, if
it isn't already, because Emacs doesn't do that, at least not by
default.  Is this convention applied consistently across the project?

> > Did we consider the option of perhaps only pushing this patch
> > to the 8.0 branch, and require MinGW 5.x for the current master?
> > If we did it that way, it would allow us to avoid remembering
> > that we need to use gdb::to_string instead of std::to_string.
> 
> That'd be fine with me, FWIW.

I wasn't sure what it means "to require MinGW 5.x", in practice, but I
pushed only to the branch for now.
  

On 05/26/2017 08:46 AM, Eli Zaretskii wrote:
>> Cc: simon.marchi@polymtl.ca, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Thu, 25 May 2017 11:12:22 +0100
>>
>> This:
>>
>>>> +	(std::to_string) [REPLACE_TO_STRING]: Provide a replacement
>>>> +	implementation.
>>
>> Should really be:
>>
>> 	(gdb::to_string): Define.
> 
> The code says std::to_string, though.  So it sounds like some coding
> conventions are being applied here of which I wasn't aware, and
> neither is Emacs.  Are these conventions described somewhere?

Just the standard GNU conventions.  The code is defining a new
function template called gdb::to_string.  Simplified:

 namespace gdb {
   template <class T> std::string to_string (const T &val);
 }

There are two implementations of that, one for mingw, written
as a new function template in place.  And another which is
importing std::to_string into the gdb namespace.  But whatever
the implementations, it's implementation detail of gdb::to_string.

So gdb::to_string is meant as a std::to_string replacement, yes.
But that's not "what", that's "why".

This is really the same as if the patch looked like this:

+ namespace gdb {
+   std::string tostr(int i)
+   {
+ #ifdef MINGW
+     // something using sprintf;
+ #else
+     return std::to_string (i);
+ #endif
+   }
+ }

The ChangeLog entry would be something like:

 	(gdb::tostr): Define.

too, not:

 	(std::to_string): Provide a replacement.

And the entry is not conditional on MINGW, since we're
adding both #ifdef and #ifndef implementations at the same
time.

> 
>> and you need an entry for the cli/cli-script.c change, like:
>>
>>         * cli/cli-script.c (user_args::insert_args): Use it.
> 
> I added that, 

Thank you.

> but once again, the convention to put the
> fully-qualified symbol name in the log entry should be documented, if
> it isn't already, because Emacs doesn't do that, at least not by
> default.  

I can't see how what Emacs does has any bearing here, since AFAIK,
Emacs isn't written in C++.  Here there's no namespace at all,
and "insert_args" is a method of the user_args class.  So I find
the above a natural a concise way to write the symbol's name.

> Is this convention applied consistently across the project?

Sure, grep ChangeLog for "::".

Thanks,
Pedro Alves
  

> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 26 May 2017 11:54:03 +0100
> 
> > The code says std::to_string, though.  So it sounds like some coding
> > conventions are being applied here of which I wasn't aware, and
> > neither is Emacs.  Are these conventions described somewhere?
> 
> Just the standard GNU conventions.  The code is defining a new
> function template called gdb::to_string.  Simplified:
> 
>  namespace gdb {
>    template <class T> std::string to_string (const T &val);
>  }
> 
> There are two implementations of that, one for mingw, written
> as a new function template in place.  And another which is
> importing std::to_string into the gdb namespace.  But whatever
> the implementations, it's implementation detail of gdb::to_string.

So the convention is to include the full qualifier of every method?
Including the namespace of the class?  Does that include the whole
chain of namespaces up to the root?  If not, where does one stop?

> > but once again, the convention to put the
> > fully-qualified symbol name in the log entry should be documented, if
> > it isn't already, because Emacs doesn't do that, at least not by
> > default.  
> 
> I can't see how what Emacs does has any bearing here, since AFAIK,
> Emacs isn't written in C++.

I use Emacs commands, such as "C-x 4 a", to generate ChangeLog
entries.  Don't you do the same?  Emacs add-log commands strip the
class and namespace qualifiers.
  

On 05/26/2017 02:02 PM, Eli Zaretskii wrote:
>> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 26 May 2017 11:54:03 +0100
>>
>>> The code says std::to_string, though.  So it sounds like some coding
>>> conventions are being applied here of which I wasn't aware, and
>>> neither is Emacs.  Are these conventions described somewhere?
>>
>> Just the standard GNU conventions.  The code is defining a new
>> function template called gdb::to_string.  Simplified:
>>
>>  namespace gdb {
>>    template <class T> std::string to_string (const T &val);
>>  }
>>
>> There are two implementations of that, one for mingw, written
>> as a new function template in place.  And another which is
>> importing std::to_string into the gdb namespace.  But whatever
>> the implementations, it's implementation detail of gdb::to_string.
> 
> So the convention is to include the full qualifier of every method?
> Including the namespace of the class?  Does that include the whole
> chain of namespaces up to the root?  If not, where does one stop?

I admit that I'm puzzled about you having an issue with
gdb::to_string, when your original entry said "(std::to_string)", 
not "(to_string)".

I think the convention is to provide enough qualifiers to
to make it convenient to search/see what function was changed.
For class methods, I think we should include the class name,
otherwise it can get ambiguous.  I'm fine with not mentioning
the namespace when otherwise it's obvious or redundant.  E.g.,
if all of gdb was in namespace gdb [it's not], it'd probably not
make much sense to keep putting "gdb::" everywhere.  But when
referring to a symbol outside gdb's namespace, I'd think it wise to
mention the namespace, so that it's clearer what the entry
is referring to.

> 
>>> but once again, the convention to put the
>>> fully-qualified symbol name in the log entry should be documented, if
>>> it isn't already, because Emacs doesn't do that, at least not by
>>> default.  
>>
>> I can't see how what Emacs does has any bearing here, since AFAIK,
>> Emacs isn't written in C++.
> 
> I use Emacs commands, such as "C-x 4 a", to generate ChangeLog
> entries.  Don't you do the same?  Emacs add-log commands strip the
> class and namespace qualifiers.

Ah, I thought you were talking about your experience as emacs
maintainer.  Sorry.

I never managed to get used to using emacs commands to write the ChangeLog.
I write ChangeLog entries manually, directly in the git commit log,
while scrolling through the diff, as last "self review" event, and
then copy over to the ChangeLog file at push time.

Thanks,
Pedro Alves
  

> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 26 May 2017 15:10:01 +0100
> 
> > So the convention is to include the full qualifier of every method?
> > Including the namespace of the class?  Does that include the whole
> > chain of namespaces up to the root?  If not, where does one stop?
> 
> I admit that I'm puzzled about you having an issue with
> gdb::to_string, when your original entry said "(std::to_string)", 
> not "(to_string)".

I did that because of the different namespaces involved in that
particular change.  But that was an exception: I normally don't look
too hard at what Emacs's add-log commands produce in the parentheses,
I just trust it to DTRT.  Having to manually correct that in each case
is a complication.

> I think the convention is to provide enough qualifiers to
> to make it convenient to search/see what function was changed.
> For class methods, I think we should include the class name,
> otherwise it can get ambiguous.  I'm fine with not mentioning
> the namespace when otherwise it's obvious or redundant.  E.g.,
> if all of gdb was in namespace gdb [it's not], it'd probably not
> make much sense to keep putting "gdb::" everywhere.  But when
> referring to a symbol outside gdb's namespace, I'd think it wise to
> mention the namespace, so that it's clearer what the entry
> is referring to.

Well, I suggest to document these conventions somewhere, like the
Wiki.  I don't think they are self-evident, and standards.texi doesn't
say anything about qualifying method names, it just tells to use the
Emacs add-log commands and the related VC features.

> I never managed to get used to using emacs commands to write the ChangeLog.
> I write ChangeLog entries manually, directly in the git commit log,
> while scrolling through the diff, as last "self review" event, and
> then copy over to the ChangeLog file at push time.

You will find some advice for a better workflow in the CONTRIBUTE file
in the Emacs repository
(http://git.savannah.gnu.org/cgit/emacs.git/tree/CONTRIBUTE).

I've just asked the CC Mode maintainer to allow the add-log commands
to not strip the class qualifiers.  But namespace qualifiers will
still have to be added by hand, I'm afraid, which is a bit of a
nuisance, IMO.
  

On 05/26/2017 03:35 PM, Eli Zaretskii wrote:

> Well, I suggest to document these conventions somewhere, like the
> Wiki.  I don't think they are self-evident, and standards.texi doesn't
> say anything about qualifying method names, it just tells to use the
> Emacs add-log commands and the related VC features.

OK.  I see plenty of entries on gcc's ChangeLogs using the same
convention, and I never thought that others would ever want to skip a
method's class name.  Agreed about wiki.  Though maybe GCC already
documents it somewhere, and we just need to point at it.  I wasn't aware
about this emacs issue.

>> I never managed to get used to using emacs commands to write the ChangeLog.
>> I write ChangeLog entries manually, directly in the git commit log,
>> while scrolling through the diff, as last "self review" event, and
>> then copy over to the ChangeLog file at push time.
> 
> You will find some advice for a better workflow in the CONTRIBUTE file
> in the Emacs repository
> (http://git.savannah.gnu.org/cgit/emacs.git/tree/CONTRIBUTE).
> 
> I've just asked the CC Mode maintainer to allow the add-log commands
> to not strip the class qualifiers.  But namespace qualifiers will
> still have to be added by hand, I'm afraid, which is a bit of a
> nuisance, IMO.
> 

Thanks.