Re: [PATCH 00/37] removal of some c++ keywords
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
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
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
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
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
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
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
[PATCH 00/37] removal of some c++ keywords
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. 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. Brandon Williams (37): object_info: change member name from 'typename' to 'type_name' object: rename function 'typename' to 'type_name' blame: rename 'this' variables pack-objects: rename 'this' variables rev-parse: rename 'this' variable diff: rename 'this' variables apply: rename 'try' variables apply: rename 'new' variables checkout: rename 'new' variables help: rename 'new' variables pack-redundant: rename 'new' variables reflog: rename 'new' variables remote: rename 'new' variables combine-diff: rename 'new' variables commit: rename 'new' variables diff-lib: rename 'new' variable diff: rename 'new' variables diffcore-delta: rename 'new' variables entry: rename 'new' variables http: rename 'new' variables imap-send: rename 'new' variables line-log: rename 'new' variables read-cache: rename 'new' variables ref-filter: rename 'new' variables remote: rename 'new' variables split-index: rename 'new' variables submodule: rename 'new' variables trailer: rename 'new' variables unpack-trees: rename 'new' variables init-db: rename 'template' variables environment: rename 'template' variables diff: rename 'template' variables environment: rename 'namespace' variables wrapper: rename 'template' variables tempfile: rename 'template' variables trailer: rename 'template' variables replace: rename 'new' variables apply.c| 106 - blame.c| 33 builtin/cat-file.c | 4 +- builtin/checkout.c | 138 - builtin/diff-tree.c| 2 +- builtin/fast-export.c | 8 +- builtin/fsck.c | 4 +- builtin/grep.c | 2 +- builtin/help.c | 10 +-- builtin/index-pack.c | 12 +-- builtin/init-db.c | 30 +++ builtin/merge.c| 2 +- builtin/mktree.c | 4 +- builtin/pack-objects.c | 8 +- builtin/pack-redundant.c | 48 ++-- builtin/prune.c| 2 +- builtin/reflog.c | 8 +- builtin/remote.c | 44 +-- builtin/replace.c | 28 +++ builtin/rev-parse.c| 34 builtin/tag.c | 2 +- builtin/unpack-objects.c | 10 +-- builtin/verify-commit.c| 2 +- bulk-checkin.c | 2 +- cache.h| 4 +- combine-diff.c | 12 +-- commit.c | 20 ++--- contrib/examples/builtin-fetch--tool.c | 2 +- diff-lib.c | 20 ++--- diff.c | 38 - diffcore-delta.c | 16 ++-- entry.c| 40 +- environment.c | 24 +++--- fast-import.c | 16 ++-- fsck.c | 2 +- git-compat-util.h | 4 +- http-push.c| 2 +- http.c | 10 +-- imap-send.c| 14 ++-- line-log.c | 48 ++-- log-tree.c | 2 +- object.c | 6 +- object.h | 2 +- pack-check.c | 2 +- packfile.c | 8 +- reachable.c| 2 +- read-cache.c | 36 - ref-filter.c | 20 ++--- remote.c | 16 ++-- sequencer.c| 2 +- sha1_file.c| 28 +++ sha1_name.c| 6 +- split-index.c | 10 +-- split