Re: bison-20001221

[Akim Demaille]

>>>>> "Hans" == Hans Aberg <address@hidden> writes:

Hans> I picked down
Hans> ftp://alpha.gnu.org/gnu/cvs/bison-20001221.tgz. Some bugs: - The
Hans> file version.texi, needed by bison.texinfo is not in the
Hans> distribution.

Well, I supposed they were building the whole distro, but you proved
they didn't.  I'm in favor of putting the whole dist under CVS, which
was not the choice of Jesse.  Jesse, any problem if I include the full
tarball under CVS?

Hans> - On a number of places there is include <config.h> It should be
Hans> include "config.h"

I never quite understood the difference between the two, and some
people will tell you to do just the opposite (see the Autoconf doc for
a start).

So I don't care :)

Hans> - Files "files.c" and "complain.c" reads the macro definition
Hans> differently: "files.c" somehow gets it from "error.h", I think,
Hans> whereas "complain.c" does not read it at all. So I had to throw
Hans> in #ifndef __attribute__ #define __attribute__(x) #endif into
Hans> the file "complain.h" in order to be sure is always defined
Hans> (which should be empty on my system).

Err?  What macro definition?

Hans> - File "files.c" uses stpcpy() and strndup() which is not in the
Hans> C standard (and I have no other mentioning of it). So I put in
Hans> definitions (hope it is correct): char* strndup(const char* s,
Hans> size_t n); char* stpcpy(char* s1, const char* s2);

Yeah, I know, currently CVS is portable to ultra comfortable
environments.  This is because there are still choices I'm not sure
of.  Of course this kind of portability details will be fixed in the
releases, but currently I'm more interested in releasing Autoconf,
then we'll address Bison.

Hans> - File "xstrdup.c", "xmalloc.c", and others, contains platform
Hans> specific seemingly unneeded #include <sys/types.h>

Fetched from elsewhere, and I trust the guy who wrote them.

Hans> - In files "output.c", output_headers(), and "print.c",
Hans> print_grammar(), I got problems with the fact that pre-MacOS X
Hans> only allows stack allocations <= 32 kB, here exceeded by the
Hans> "obstack_..." macros. 

Too bad...  Seriously, I won't address this issue.  If you want to do
something, fix obstack and send the patches to the Glibc guys.  Be
ready to fight if you really want to have MacOS stuff included in
there :)

Hans> This just something platform specific of an old OS, but I think
Hans> in general, it is wise to not allocate more on the stack than
Hans> needed. (For example, each obstack_fgrow1 asks for a stack
Hans> allocation of 4 kB, whereas in output_headers() the actual
Hans> allocation needed for each obstack_fgrow1 is less than 64 B.)

I'm really surprised.  What makes you think that the stack is
involved?  Aren't you referring to the heap instead?  Obstack is
no more aggressive with the stack than malloc is.

Hans> - I got around the problem by changing the obstack_fgrow1 macro
Hans> containing a static array: static char buf[4096]; Not pure
Hans> anymore, but it does not make any difference in my port.

Aaaaah!  So you are talking about _my_ macros (see system.h).  Well,
your compiler is dumb: at no time several buffers of 4096 are
concurrently used.  Complain to the author of the compiler explaining
that objects with non overlapping lifetimes can share the same storage.

Hans> - Files "quote.c", "quote_arg.c" includes platform specific
Hans> <sys/types.h>.  

So what?

Hans> Unnecessary if "quotearg.h" includes <size_t.h>. 

???  No it does not.

Hans> Also, #include <quotearg.h> #include <quote.h> only works if one
Hans> puts the stuff in the library; not if one takes down Bison and
Hans> compiles it directly; #include "quotearg.h" #include "quote.h"
Hans> is safer.

I don't understand what issue you address here.  It compiles fine in
here, and that's all I'm asking for.  I'm importing these files from
elsewhere and will not make them Bison specific if that's what you mean.

Hans> - File "quote_arg.c", "else" clause of "#if HAVE_MBRTOWC": macro
Hans> "iswprint" is already in the C-standard, so redefining it here
Hans> causes an error.

This is good to know.  Please let me see the error.

Hans> - File "main.c", main() does not have a return value -- produces
Hans> a warning from the compiler.

Thanks.

Hans> - File "bison.simple", int yyparse (YYPARSE_PARAM_ARG)
Hans> YYPARSE_PARAM_DECL does not have a prototype -- produces a
Hans> compiler warning for example when compiling under C++.

Yeah, but for the time being I don't care.  Sorry about this, I truly
am, but we have to set priorities.

Hans> - The new dynamically computed #line directives are great. --
Hans> The filename, however show up with a full pathname. Perhaps some
Hans> may want that, but suppose the sources are distributed to
Hans> another computer with another filepath -- then that is
Hans> broken. So I suggest instead if the single "char* skeleton",
Hans> which currently full path-name, using two "skeleton_name" and
Hans> "skeleton_path"; then one can use "skeleton_name" in the #line
Hans> directives.  - I realize when I write this that it probably
Hans> happens because I tweaked "skeleton_find" to output the full
Hans> pathname. I therefore tweaked it back in output_parser().

The current solution seems right to me: it is neither absolute, nor
relative etc.  it is just what the user wrote.  It must remain as is.


Thanks for all these suggestions, Hans.