* Merging ‘bdw-gc-static-alloc’ @ 2009-10-02 9:15 Ludovic Courtès 2009-10-02 16:26 ` Andy Wingo 0 siblings, 1 reply; 8+ messages in thread From: Ludovic Courtès @ 2009-10-02 9:15 UTC (permalink / raw) To: guile-devel Hello! Following the inlining of stringbuf storage (ba54a2026beaadb4e7566d4b9e2c9e4c7cd793e6), the ‘bdw-gc-static-alloc' branch introduces a small source-level incompatibility. Namely, the following no longer works: const char foo[] = "foo"; SCM_SYMBOL (s_foo, foo); Because of the macrology that’s used to statically allocate stringbufs [0], the string has to be a literal: SCM_SYMBOL (s_foo, "foo"); The use of non-literal strings occurred only in ‘eval.c’ in Guile proper [1]. The question is: should we merge ‘bdw-gc-static-alloc’ despite this incompatibility (it can be argued that this won’t hurt many people and can be easily worked around)? If the answer is “no”, we still have the option of merging other aspects of the ‘bdw-gc-static-alloc’ branch, such as static allocation of subrs. What do you think? Thanks, Ludo’. [0] http://git.sv.gnu.org/gitweb/?p=guile.git;a=blobdiff;f=libguile/snarf.h;h=9eaccf60c14d825e657718e6f75257e293649f84;hp=c3113e1a7a977f95eb922e924855244dfe4d6cc2;hb=5f236208d0d864546e59afa0f5a11c9b3ba14b10;hpb=f0eb5ae6c173aed35965b0561897fda1d8ff0db1 [1] http://git.sv.gnu.org/gitweb/?p=guile.git;a=blobdiff;f=libguile/eval.c;h=e58c05410c2831d4061eb92b4ee2883c9203c919;hp=59db42976242e12fd41995e5e1fa5d169bd3b9bf;hb=5f236208d0d864546e59afa0f5a11c9b3ba14b10;hpb=d7e7a02a6251c8ed4f76933d9d30baeee3f599c0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Merging ‘bdw-gc-static-alloc’ 2009-10-02 9:15 Merging ‘bdw-gc-static-alloc’ Ludovic Courtès @ 2009-10-02 16:26 ` Andy Wingo 2009-10-23 9:16 ` Ludovic Courtès 0 siblings, 1 reply; 8+ messages in thread From: Andy Wingo @ 2009-10-02 16:26 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guile-devel On Fri 02 Oct 2009 02:15, ludo@gnu.org (Ludovic Courtès) writes: > Because of the macrology that’s used to statically allocate stringbufs [0], > the string has to be a literal: > > SCM_SYMBOL (s_foo, "foo"); > > What do you think? Need more information :) How widespread is the non-literal usage? Could you grep the source packages of the Guile users that you have identified for occurrences of this idiom? Andy -- http://wingolog.org/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Merging ‘bdw-gc-static-alloc’ 2009-10-02 16:26 ` Andy Wingo @ 2009-10-23 9:16 ` Ludovic Courtès 2009-10-23 12:57 ` Andy Wingo 0 siblings, 1 reply; 8+ messages in thread From: Ludovic Courtès @ 2009-10-23 9:16 UTC (permalink / raw) To: guile-devel Hi, Andy Wingo <wingo@pobox.com> writes: > On Fri 02 Oct 2009 02:15, ludo@gnu.org (Ludovic Courtès) writes: > >> Because of the macrology that’s used to statically allocate stringbufs [0], >> the string has to be a literal: >> >> SCM_SYMBOL (s_foo, "foo"); >> >> What do you think? > > Need more information :) How widespread is the non-literal usage? Could > you grep the source packages of the Guile users that you have identified > for occurrences of this idiom? Apparently this is going to be rare. I’ve found no occurrences here: http://www.google.fr/codesearch?q=SCM_%28GLOBAL_%29%3FSYMBOL&hl=fr&btnG=Rechercher+du+code “grep -r -E \ 'SCM_(GLOBAL_)?SYMBOL[[:blank:]]*\([^,]+,[[:blank:]]*[^"]+\)' \ gnucash-2.2.9 guile-avahi guile-cairo-1.4.0 \ guile-gnome-platform-2.16.1 gnutls mailutils-2.1 snd-9.4” doesn’t show any occurrence either. So I’d be tempted to merge this in. What do you think? Ludo’. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Merging ‘bdw-gc-static-alloc’ 2009-10-23 9:16 ` Ludovic Courtès @ 2009-10-23 12:57 ` Andy Wingo 2009-11-02 21:44 ` Ludovic Courtès 0 siblings, 1 reply; 8+ messages in thread From: Andy Wingo @ 2009-10-23 12:57 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guile-devel EHLO On Fri 23 Oct 2009 11:16, ludo@gnu.org (Ludovic Courtès) writes: > Andy Wingo <wingo@pobox.com> writes: > >> On Fri 02 Oct 2009 02:15, ludo@gnu.org (Ludovic Courtès) writes: >> >>> Because of the macrology that’s used to statically allocate stringbufs [0], >>> the string has to be a literal: >>> >>> SCM_SYMBOL (s_foo, "foo"); >>> >>> What do you think? >> >> Need more information :) How widespread is the non-literal usage? Could >> you grep the source packages of the Guile users that you have identified >> for occurrences of this idiom? > > Apparently this is going to be rare. I’ve found no occurrences here: > > So I’d be tempted to merge this in. > > What do you think? Roll on, merge train, roll on! I have finished case-lambda, but am rebasing and cleaning up patches I haven't pushed yet. So it's a race. Gentlemen, start your git rebase & push! A -- http://wingolog.org/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Merging ‘bdw-gc-static-alloc’ 2009-10-23 12:57 ` Andy Wingo @ 2009-11-02 21:44 ` Ludovic Courtès 2009-11-03 20:57 ` (no subject) Neil Jerram 0 siblings, 1 reply; 8+ messages in thread From: Ludovic Courtès @ 2009-11-02 21:44 UTC (permalink / raw) To: guile-devel [-- Attachment #1: Type: text/plain, Size: 592 bytes --] Hello! I merged that branch in ‘master’. ... and soon after I noticed this glitch with ‘SCM_DEFINE’: when ‘SCM_SUPPORT_STATIC_ALLOCATION’ is defined, it requires a previous C declaration of the subr being defined. For instance: SCM_DEFINE (foo, "foo", 1, 0, 0, (SCM x), "") { ... } must now be preceded by: extern SCM foo (SCM); This is not a problem in Guile core where we compile with ‘-Wmissing-prototypes’ and provide public declarations for all ‘SCM_DEFINE’d functions, but it may be a problem in projects that don’t. This is easily remedied this way: [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Type: text/x-patch, Size: 624 bytes --] --- a/libguile/snarf.h +++ b/libguile/snarf.h @@ -101,14 +101,15 @@ SCM_SNARF_DOCS(primitive, FNAME, PRIMNAME, ARGLIST, REQ, OPT, VAR, DOCSTRING) /* Static subr allocation. */ #define SCM_DEFINE(FNAME, PRIMNAME, REQ, OPT, VAR, ARGLIST, DOCSTRING) \ SCM_SYMBOL (scm_i_paste (FNAME, __name), PRIMNAME); \ -SCM_SNARF_HERE( \ +SCM_SNARF_HERE( \ static const char scm_i_paste (s_, FNAME) [] = PRIMNAME; \ + extern SCM FNAME ARGLIST; \ SCM_IMMUTABLE_SUBR (scm_i_paste (FNAME, __subr), \ scm_i_paste (FNAME, __name), \ REQ, OPT, VAR, &FNAME); \ SCM FNAME ARGLIST \ [-- Attachment #3: Type: text/plain, Size: 679 bytes --] However, there are potentially 2 problems with this: 1. The declaration could conflict with a previous, slightly different one. Apparently, a given function declaration and the same one decorated with GCC function attributes are considered the same, so this should be OK. However, I’m slightly concerned about MSVC’s __declspec: does it work if it sees: __declspec(dllexport) extern SCM foo (SCM); extern SCM foo (SCM); If it works, that probably means this point is moot. 2. The automatically added ‘extern’ declaration makes ‘-Wmissing-prototypes’ useless, which is annoying. Thoughts? Thanks, Ludo’. ^ permalink raw reply [flat|nested] 8+ messages in thread
* (no subject) 2009-11-02 21:44 ` Ludovic Courtès @ 2009-11-03 20:57 ` Neil Jerram 2009-11-05 22:30 ` Merging ‘bdw-gc-static-alloc’ Ludovic Courtès 0 siblings, 1 reply; 8+ messages in thread From: Neil Jerram @ 2009-11-03 20:57 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guile-devel ludo@gnu.org (Ludovic Courtès) writes: > should be OK. However, I’m slightly concerned about MSVC’s > __declspec: does it work if it sees: > > __declspec(dllexport) extern SCM foo (SCM); > extern SCM foo (SCM); I don't know for sure, but I know I've seen "function redeclared with different linkage" warnings in the past, and this could well be one of the situations that generates such warnings. But what about using SCM_API instead of extern? Wouldn't that always generate the right code? (At least for building guile itself.) > If it works, that probably means this point is moot. > > 2. The automatically added ‘extern’ declaration makes > ‘-Wmissing-prototypes’ useless, which is annoying. I guess it depends if the primitives in question are designed to be used only from Scheme, or also from other C files. If they're only for Scheme, it's a nice feature that SCM_DEFINE does everything needed and the developer doesn't need a matching prototype. If they're for other C files, the missing prototype warning could be helpful. Then again, the developer will get some kind of warning anyway when trying to use the function from another file. So I think the balance is in favour of SCM_DEFINE including the prototype. Neil ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Merging ‘bdw-gc-static-alloc’ 2009-11-03 20:57 ` (no subject) Neil Jerram @ 2009-11-05 22:30 ` Ludovic Courtès 2009-11-08 22:21 ` (no subject) Neil Jerram 0 siblings, 1 reply; 8+ messages in thread From: Ludovic Courtès @ 2009-11-05 22:30 UTC (permalink / raw) To: Neil Jerram; +Cc: guile-devel Hi Neil, Neil Jerram <neil@ossau.uklinux.net> writes: > ludo@gnu.org (Ludovic Courtès) writes: > >> should be OK. However, I’m slightly concerned about MSVC’s >> __declspec: does it work if it sees: >> >> __declspec(dllexport) extern SCM foo (SCM); >> extern SCM foo (SCM); > > I don't know for sure, but I know I've seen "function redeclared with > different linkage" warnings in the past, and this could well be one of > the situations that generates such warnings. Right. > But what about using SCM_API instead of extern? Wouldn't that always > generate the right code? (At least for building guile itself.) You’re right. It’ll work for building Guile itself, but most probably fail when building external libraries, because it will have SCM_API==dllimport whereas the symbols of the lib being built want dllexport. I have no idea how to fix this since third party libraries typically have their own ‘SCM_API’-like macro for that purpose. Well, we could just disable ‘SCM_SUPPORT_STATIC_ALLOCATION’ altogether for “Woe32” builds. What do you think? In the meantime, I’ve pushed this: http://git.savannah.gnu.org/cgit/guile.git/commit/?id=f4767979987530bb9d88dde6ebe61cdac6f756b9 . > I guess it depends if the primitives in question are designed to be used > only from Scheme, or also from other C files. If they're only for > Scheme, it's a nice feature that SCM_DEFINE does everything needed and > the developer doesn't need a matching prototype. If they're for other C > files, the missing prototype warning could be helpful. Yes, although ideally functions only for Scheme would be either static or with internal linkage, which calls for an ‘SCM_DEFINE_LOCAL’ macro or similar. > Then again, the developer will get some kind of warning anyway when > trying to use the function from another file. So I think the balance is > in favour of SCM_DEFINE including the prototype. Yes. Thanks, Ludo’. ^ permalink raw reply [flat|nested] 8+ messages in thread
* (no subject) 2009-11-05 22:30 ` Merging ‘bdw-gc-static-alloc’ Ludovic Courtès @ 2009-11-08 22:21 ` Neil Jerram 0 siblings, 0 replies; 8+ messages in thread From: Neil Jerram @ 2009-11-08 22:21 UTC (permalink / raw) To: Ludovic Courtès; +Cc: guile-devel ludo@gnu.org (Ludovic Courtès) writes: > Hi Neil, > > Neil Jerram <neil@ossau.uklinux.net> writes: > >> But what about using SCM_API instead of extern? Wouldn't that always >> generate the right code? (At least for building guile itself.) > > You’re right. It’ll work for building Guile itself, but most probably > fail when building external libraries, because it will have > SCM_API==dllimport whereas the symbols of the lib being built want > dllexport. > > I have no idea how to fix this since third party libraries typically > have their own ‘SCM_API’-like macro for that purpose. > > Well, we could just disable ‘SCM_SUPPORT_STATIC_ALLOCATION’ altogether > for “Woe32” builds. > > What do you think? We're talking here about macros that are always used in .c files, aren't we? (As opposed to headers that an application using the library would include.) So on Woe32, we actually always want dllexport. i.e. if the header has SCM_API SCM scm_frobate (SCM arg); and the SCM_DEFINE in .c expands to something that includes __declspec(dllexport) SCM scm_frobate (SCM arg); that should be good, because SCM_API will be __declspec(dllexport) when building the library. In the Scheme-only case, that does mean exporting something that doesn't need exporting - but either we can just live with that, or later add your suggestion of SCM_DEFINE_LOCAL. Regards, Neil ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-11-08 22:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-10-02 9:15 Merging ‘bdw-gc-static-alloc’ Ludovic Courtès 2009-10-02 16:26 ` Andy Wingo 2009-10-23 9:16 ` Ludovic Courtès 2009-10-23 12:57 ` Andy Wingo 2009-11-02 21:44 ` Ludovic Courtès 2009-11-03 20:57 ` (no subject) Neil Jerram 2009-11-05 22:30 ` Merging ‘bdw-gc-static-alloc’ Ludovic Courtès 2009-11-08 22:21 ` (no subject) Neil Jerram
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).