Hi Eric, thanks for the quick review.
* Eric Blake wrote on Wed, Jun 04, 2008 at 10:19:40PM CEST: > Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de> writes: > > # Replace #undef with comments. This is necessary, for example, > > # in the case of _POSIX_SOURCE, which is predefined and required > > # on some systems where configure will not decide to define it. > > > > (maybe we should document that?) > > We already mention the aspect of commenting out mentioned but unused #undef > lines, earlier in the same node: > > | Then you could have code like the following in `conf.h.in'. On systems > | that have `unistd.h', `configure' defines `HAVE_UNISTD_H' to 1. On > | other systems, the whole line is commented out (in case the system > | predefines that symbol). Oh, thanks, I overlooked that. > > cat >config.h.in <<\EOF > > #undef DEFINED_SYMBOL /* some comment */ > > #undef UNDEFINED_SYMBOL /* some other comment */ > > #undef DEFINED_SYMBOL > > #undef UNDEFINED_SYMBOL > > EOF > > This test case would have been nicer with three cases instead of two: > DEFINED_SYMBOL encountered in AC_DEFINE and substituted during configure > UNDEFINED_SYMBOL mentioned in AC_DEFINE, but not encountered during configure > OTHER_SYMBOL not mentioned at all in configure.ac I tried OTHER_SYMBOL; it behaves exactly the same as UNDEFINED_SYMBOL. > If I understand the bug report correctly, maybe we should try to tweak things > such that if any AC_DEFINE mentions the hook, we comment out that hook when > not > substituted, but if the symbol is not mentioned at all, we don't treat it as > an > undefined hook. On the other hand, I don't know how complex this would be to > implement, now that we rewrote the config.status algorithm to use awk instead > of sed. FWIW, we never did that before. But the move to awk certainly made this a lot easier, with sed it would've been almost impossible. > Meanwhile, you can work around this by using AH_VERBATIM or AT_BOTTOM to > #include a secondary file, and place your #undef in that second file without > worrying whether autoheader/config.status will munge it. So blindly > commenting > ALL #undef in the template file, whether or not they are mentioned as hooks > in > configure.ac, is not an unsurmountable limitation. Agreed. > So I think the best thing to do now would be a wording tweak to make it > obvious > that we intentionally comment out ALL remaining #undef in the template file, > rather than just the ones that were hooked by AC_SUBST/AC_DEFINE but not > substituted. Or fix it. > > --- 2.13 --- > > /* config.h. Generated automatically by configure. */ > > #define DEFINED_SYMBOL 1 /* some comment */ > > /* #undef UNDEFINED_SYMBOL */ /* some other comment */ > > #define DEFINED_SYMBOL 1 > > Is this a bug? If I remember correctly, the preprocessor is supposed to warn > if the sequence of tokens following the redefinition of an existing macro > name > is not identical to its prior definition, and I'm not sure whether the > removal > of a comment qualifies as a different sequence of tokens. I'd have to look that up; but anyway, I don't think this is a big problem, as users are unlikely to define things twice. > > --- 2.59 --- > > /* config.h. Generated by configure. */ > > /* #undef DEFINED_SYMBOL */ /* some comment */ > > /* #undef UNDEFINED_SYMBOL */ /* some other comment */ > > #define DEFINED_SYMBOL 1 > > /* #undef UNDEFINED_SYMBOL */ > > Thus I like this output slightly better - no risk of redefining an already- > defined symbol with a different sequence of tokens. Well, no. The use case is that only the first line exists; in that case, autoconf output is buggy. Maybe my test case was not good; there is never any state saved across lines; so, the second substitution does not remember any previous ones. > > --- 2.61 --- > > /* config.h. Generated from config.h.in by configure. */ > > #undef DEFINED_SYMBOL /* some comment */ > > This approach of leaving all but the last occurence of a defined hook as an > #undef would also avoid redefining a symbol... It's not the last one; it's that presence of the comment prevents a successful substitution. > > #undef UNDEFINED_SYMBOL /* some other comment */ > > ...but leaving the first instance of an undefined hook uncommented is indeed > a > bug. On the other hand, template files generally don't mention the same hook > multiple times. Exactly. > > #define DEFINED_SYMBOL 1 > > /* #undef UNDEFINED_SYMBOL */ > > Question is if that's good enough. Judging from autoconf history as > > seen above, comments have been problematic ever since 2.59. I haven't > > tried incomplete (multi-line) comments, or C++ style comments. > > With C++ comments, it should not matter whether we follow the 2.13 approach > of > inserting */ after the macro name and before the rest of the line, or your > proposed patch of truncation, since all of these are correctly commented: > /* #undef foo */ // comment > /* #undef foo // comment */ > /* #undef foo */ > > But if I understand things correctly, multi-line comments suffer the same > fate > in 2.62 (except that the first */ comes from the config.status substitution, > and the second from the template, rather than in the one-line case where the > first */ is from the template). > > #undef foo /* one > two */ > > => > > #undef foo /* one */ > two */ > > For this case, then, the 2.13/2.59 approach of putting the closing */ after > the > macro name but prior to all other contents of the line is better than > truncating the line after the macro name and first */ (truncating the line > will > leave the incomplete comment conclusion on the next line): Agreed. > On the other hand, only the 2.62 behavior and your proposed truncation > handles > the case of the user doing invalid preprocessor constructs in the template, > such as this: > > #undef foo not valid We should not strive to support that. That's a user bug. If we need to make that clearer, then we should do so. > > - print "/*", line, "*/" > > + print "/*", prefix defundef, macro, "*/" > > How hard would it be to split the original line into the macro name and the > rest of the line, such that we output > > /* #undef foo */ /* comment */ > > similar to 2.13? I'll look into it. Cheers, Ralf
