Re: [PATCH 00/37] removal of some c++ keywords

[Duy Nguyen]

On Wed, Jan 31, 2018 at 7:57 AM, Stefan Beller  wrote:
>> There's also C99 designator in builtin/clean.c (I thought we avoided
>> C99, I can start using this specific feature more now :D)
>
> That was a test balloon? See 512f41cfac
> (clean.c: use designated initializer, 2017-07-14)

Aww.. I thought it was in there since forever and it should be safe to
use now...

> One of the big advantages would be stricter type checking, such as
> signed/unsigned confusion, that we occasionally have.
> e.g. 61d36330b4 (prefer "!=" when checking read_in_full()
> result, 2017-09-27) or what is referenced from there 561598cfcf
> (read_pack_header: handle signed/unsigned comparison in read result,
> 2017-09-13).

We can do that even with C (at least with gcc and I guess clang as
well). The problem is it looks so bad right now that I have to turn it
off with -Wno-sign-compare

> The bugs resulting in these patches could have been caught more easily
> with C++ checking IMHO.
-- 
Duy

Re: [PATCH 00/37] removal of some c++ keywords

[Stefan Beller]

On Tue, Jan 30, 2018 at 4:48 PM, Duy Nguyen  wrote:
> On Wed, Jan 31, 2018 at 6:01 AM, Stefan Beller  wrote:
>> On Tue, Jan 30, 2018 at 2:36 PM, Junio C Hamano  wrote:
>>> Duy Nguyen  writes:
>>>
 Is it simpler (though hacky) to just  do

 #ifdef __cplusplus
 #define new not_new
 #define try really_try
 ...

 somewhere in git-compat-util.h?
>>>
>>> Very tempting, especially given that your approach automatically
>>> would cover topics in flight without any merge conflict ;-)
>>>
>>> I agree that it is hacky and somewhat ugly, but the hackiness
>>> somehow does not bother me too much in this case; perhaps because
>>> attempting to use a C++ compiler may already be hacky in the first
>>> place?
>>>
>>> It probably depends on the reason why we are doing this topic.  If a
>>> report about our source code coming from the C++ oriented tool cite
>>> the symbol names seen by machines, then the "hacky" approach will
>>> give us "not_new" where Brandon's patch may give us "new_oid", or
>>> whatever symbol that is more appropriate for the context it appears
>>> than such an automated cute name.
>
> Well. the world after post processing is always ugly. But we could try
> "#define new new__" to get the not so ugly names. new_oid is
> definitely better regardless of c/c++ though so I could see that as a
> good cleanup.
>
 Do we use any C features that are incompatible with C++? (or do we not
 need to care?)
>>>
>>> Good question.
>>
>> implicit casts from void?
>> e.g. xmalloc returns a void pointer, not the type requested.
>> https://embeddedartistry.com/blog/2017/2/28/c-casting-or-oh-no-we-broke-malloc
>
> That causes lots of warnings but not errors (I bit the bullet and
> tried to compile git with g++).

And for g++ there is a flag to disable this specific set of warnings.
I think the value of using C++ analysis tools is in the LLVM/clang
world, not GNU.

> The next set of changes would be to
> reorganize nested enum/struct declarations. Even if nested, C
> considers these flat while C++ sees them in namespaces. There's some
> warnings about confusion with the new cool feature string literals,
> but that's easy to fix.
>
> There's also C99 designator in builtin/clean.c (I thought we avoided
> C99, I can start using this specific feature more now :D)

That was a test balloon? See 512f41cfac
(clean.c: use designated initializer, 2017-07-14)

One of the big advantages would be stricter type checking, such as
signed/unsigned confusion, that we occasionally have.
e.g. 61d36330b4 (prefer "!=" when checking read_in_full()
result, 2017-09-27) or what is referenced from there 561598cfcf
(read_pack_header: handle signed/unsigned comparison in read result,
2017-09-13).

The bugs resulting in these patches could have been caught more easily
with C++ checking IMHO.

> I was stuck at the thread_local thing in index-pack.c and gave up. So
> I don't know what else we would need to change.

Thanks for experimenting!
Stefan

> --
> Duy

Re: [PATCH 00/37] removal of some c++ keywords

[Duy Nguyen]

On Wed, Jan 31, 2018 at 6:01 AM, Stefan Beller  wrote:
> On Tue, Jan 30, 2018 at 2:36 PM, Junio C Hamano  wrote:
>> Duy Nguyen  writes:
>>
>>> Is it simpler (though hacky) to just  do
>>>
>>> #ifdef __cplusplus
>>> #define new not_new
>>> #define try really_try
>>> ...
>>>
>>> somewhere in git-compat-util.h?
>>
>> Very tempting, especially given that your approach automatically
>> would cover topics in flight without any merge conflict ;-)
>>
>> I agree that it is hacky and somewhat ugly, but the hackiness
>> somehow does not bother me too much in this case; perhaps because
>> attempting to use a C++ compiler may already be hacky in the first
>> place?
>>
>> It probably depends on the reason why we are doing this topic.  If a
>> report about our source code coming from the C++ oriented tool cite
>> the symbol names seen by machines, then the "hacky" approach will
>> give us "not_new" where Brandon's patch may give us "new_oid", or
>> whatever symbol that is more appropriate for the context it appears
>> than such an automated cute name.

Well. the world after post processing is always ugly. But we could try
"#define new new__" to get the not so ugly names. new_oid is
definitely better regardless of c/c++ though so I could see that as a
good cleanup.

>>> Do we use any C features that are incompatible with C++? (or do we not
>>> need to care?)
>>
>> Good question.
>
> implicit casts from void?
> e.g. xmalloc returns a void pointer, not the type requested.
> https://embeddedartistry.com/blog/2017/2/28/c-casting-or-oh-no-we-broke-malloc

That causes lots of warnings but not errors (I bit the bullet and
tried to compile git with g++). The next set of changes would be to
reorganize nested enum/struct declarations. Even if nested, C
considers these flat while C++ sees them in namespaces. There's some
warnings about confusion with the new cool feature string literals,
but that's easy to fix.

There's also C99 designator in builtin/clean.c (I thought we avoided
C99, I can start using this specific feature more now :D)

I was stuck at the thread_local thing in index-pack.c and gave up. So
I don't know what else we would need to change.
-- 
Duy

Re: [PATCH 00/37] removal of some c++ keywords

[Stefan Beller]

On Tue, Jan 30, 2018 at 2:36 PM, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>> Is it simpler (though hacky) to just  do
>>
>> #ifdef __cplusplus
>> #define new not_new
>> #define try really_try
>> ...
>>
>> somewhere in git-compat-util.h?
>
> Very tempting, especially given that your approach automatically
> would cover topics in flight without any merge conflict ;-)
>
> I agree that it is hacky and somewhat ugly, but the hackiness
> somehow does not bother me too much in this case; perhaps because
> attempting to use a C++ compiler may already be hacky in the first
> place?
>
> It probably depends on the reason why we are doing this topic.  If a
> report about our source code coming from the C++ oriented tool cite
> the symbol names seen by machines, then the "hacky" approach will
> give us "not_new" where Brandon's patch may give us "new_oid", or
> whatever symbol that is more appropriate for the context it appears
> than such an automated cute name.
>
>> Do we use any C features that are incompatible with C++? (or do we not
>> need to care?)
>
> Good question.

implicit casts from void?
e.g. xmalloc returns a void pointer, not the type requested.
https://embeddedartistry.com/blog/2017/2/28/c-casting-or-oh-no-we-broke-malloc

Re: [PATCH 00/37] removal of some c++ keywords

[Junio C Hamano]

Duy Nguyen  writes:

> Is it simpler (though hacky) to just  do
>
> #ifdef __cplusplus
> #define new not_new
> #define try really_try
> ...
>
> somewhere in git-compat-util.h?

Very tempting, especially given that your approach automatically
would cover topics in flight without any merge conflict ;-)

I agree that it is hacky and somewhat ugly, but the hackiness
somehow does not bother me too much in this case; perhaps because
attempting to use a C++ compiler may already be hacky in the first
place?

It probably depends on the reason why we are doing this topic.  If a
report about our source code coming from the C++ oriented tool cite
the symbol names seen by machines, then the "hacky" approach will
give us "not_new" where Brandon's patch may give us "new_oid", or
whatever symbol that is more appropriate for the context it appears
than such an automated cute name.

> Do we use any C features that are incompatible with C++? (or do we not
> need to care?)

Good question.

Re: [PATCH 00/37] removal of some c++ keywords

[Johannes Sixt]

Am 29.01.2018 um 23:36 schrieb Brandon Williams:

A while back there was some discussion of getting our codebase into a state
where we could use a c++ compiler if we wanted to (for various reason like
leveraging c++ only analysis tools, etc.).  Johannes Sixt had a very large
patch that achieved this but it wasn't in a state where it could be upstreamed.
I took that idea and did some removals of c++ keywords (new, template, try,
this, etc) but broke it up into several (well maybe more than several) patches.
I don't believe I've captured all of them in this series but this is at least
moving a step closer to being able to compile using a c++ compiler.


Cool! The patches all look reasonable. Some keywords remain: 'delete', 
'private', 'thread_local', 'not', 'xor', 'explicit', 'export'.



I don't know if this is something the community still wants to move towards,
but if this is something people are still interested in, and this series is
wanted, then we can keep doing these sort of conversions in chunks slowly.


I've rebased my patches on top of this series. They are available from

  https://github.com/j6t/git.git c-plus-plus

-- Hannes

Re: [PATCH 00/37] removal of some c++ keywords

[Duy Nguyen]

On Tue, Jan 30, 2018 at 5:36 AM, Brandon Williams  wrote:
> A while back there was some discussion of getting our codebase into a state
> where we could use a c++ compiler if we wanted to (for various reason like
> leveraging c++ only analysis tools, etc.).  Johannes Sixt had a very large

I would be more convinced if I knew exactly what leverage we could
have here (e.g. what tool, how many problems it caught...).

> patch that achieved this but it wasn't in a state where it could be 
> upstreamed.
> I took that idea and did some removals of c++ keywords (new, template, try,
> this, etc) but broke it up into several (well maybe more than several) 
> patches.
> I don't believe I've captured all of them in this series but this is at least
> moving a step closer to being able to compile using a c++ compiler.

Is it simpler (though hacky) to just  do

#ifdef __cplusplus
#define new not_new
#define try really_try
...

somewhere in git-compat-util.h?

Do we use any C features that are incompatible with C++? (or do we not
need to care?)

> I don't know if this is something the community still wants to move towards,
> but if this is something people are still interested in, and this series is
> wanted, then we can keep doing these sort of conversions in chunks slowly.

You're going to need to setup C++ build job on Travis or something to
catch new C++ keywords from entering the code base as well if you move
this to the end.
-- 
Duy