On 05/06/2013 03:05 PM, David Malcolm wrote:
Note that this code is for the gengtype build-time utility, rather than
in GCC proper, so this wasn't on my own mental hitlist for fixing global
state.
Yea, but we probably should be taking opportunities to clean this stuff
up even in the gen* utilities when we can.
The attached rewrite of the patch introduces classes to hold the new
internal state relating to writing out the s-expressions, and by doing
so implicitly adds a requirement that this code be compiled with C++
(I'm not sure that such a requirement existed before).
The introduction of C++ as a requirement for the gen* programs should be
OK as long as we stick to the guidelines posted on the website.
The new variables become data members of a new s_expr_writer class (and
as such gain trailing underscores as per
http://gcc.gnu.org/codingconventions.html#Cxx_Names )
Right.
It does not remove all global state from the writer code, merely the new
state that the old version of the patch added. For example:
FILE * state_file
is still global (and shared by the reader code), but fixing that would
make the patch touch many more lines of code and thus be more difficult
to read.
Yea, I wasn't asking you to go back and remove all the state. At least
with your patch we have somewhere to hang the state, which makes a nice
cleanup project for someone.
I believe I already fixed these in
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00259.html
(again, is there an automated way of checking this?)
I'm not aware of an automated way right now. Probably the closest would
be to use gnu-indent. The problem is the sources aren't already in
gnu-indent form.
That's the kind of thing I'd really like to see automated via commit
hooks. Declare gnu-indent as the standard form, then ensure the tree is
always in that form.
Anyway, the patch is good to go. Please install.
Thanks,
jeff
