Print

Please login or register.
1 Hour 1 Day 1 Week 1 Month Forever
Login with username, password and session length

[SuicideJunkie]

As of updating from the repo tonight, I am unable to build LCS:

Code: [Select]

In file included from ./includes.h:859:0,
                 from game.cpp:68:
./log/log.h:156:9: error: variable or field ‘cur_term’ declared void
./log/log.h:156:9: error: expected ‘;’ at end of member declaration
./log/log.h:156:9: error: expected unqualified-id before ‘->’ token
Searching the entire directory tree doesn't turn up the word "cur_term" at all, and there are no uncommented arrows in the file, so I'm quite confused.

Any idea what might be going on here?

PS:
The problem starts in revision 816, which changes most of the files, unfortunately.

PPS:
I haven't narrowed down where the actual cause is, but adding "#undef newline" to the log.h code makes things work again.
Apparently it is getting defined as newline="cur_term->type. Strings[103]" somewhere, but a search on the root directory turns up nothing.

[Jonathan S. Fox]

I believe the problem is term.h as implemented in ncurses. It can't be included with most of the project because of a namespace conflict with our logging system.

I'm not able to test this fix myself since I'm not compiling with ncurses, but try moving these lines out of common.h and into consolesupport.cpp, just below the include for <externs.h>:

Code: [Select]

#ifdef NCURSES
#include <term.h>
#endif

[SuicideJunkie]

Looks like it is working much better now.

[Liberal Elitist]

Thanks for the report, obviously this is my bad (as you noted this appeared in revision 816 and you can clearly see I'm the one who committed that revision). As for why so many files were changed and this problem happened, it is quite simple, I overhauled the #includes and debug defines so that all sorts of duplicative code in different files (for instance, multiple different header and even source files that included the curses libraries and had to do different tests to see whether to include PDcurses, ncurses, or xcurses, and which had to test for which operating system you were compiling on and include different libraries depending on that, etc.), this similar code appearing in multiple different files when it came to what libraries were included was not a good programming practice and likely to lead to bugs (as this duplicative code in different files wasn't quite identical in them), so I moved all that code into common.h and had it included everywhere, and changed the #include lists for other files so that, for instance, now the .cpp files only have to include one header each, either externs.h for most of them or includes.h for game.cpp because it defines all those externs listed in externs.h.

But nickdumas fixed this in revision 817, apparently using the fix Jonathan S. Fox found. Anyway term.h seems to #define a lot of stuff. I don't have term.h on my system since it's a Windows system but I could move term.h's include back into common.h with the rest of the includes of external libraries if I put in the #undef for newline, and possibly other #undefs, since it seems term.h has a whole bunch of macros with short names, at least in one version of it I found via a Google search. Hmm, I wonder if nickdumas has an account on the forums here... probably, since he committed the exact same fix that Jonathan S. Fox posted in this thread.

Anyway what I did consolidating the #includes of external libraries into common.h will lead to less bugs in the long run because now there's no longer several different slightly different long sections with #ifdefs and #includes and whatnot in several different files, all of which were pretty similar but had differences. Now since it's all done in one place, if anything goes wrong we'll all know to fix it in that one place... in common.h. And if we need to make changes they only need to be done in that one place, rather than all those separate places where these #ifdefs and #includes were done before. I do apologize for that one minor mistake... it would've worked fine if it weren't for term.h having #defines in it that override stuff used in the logging code.

The term.h code I found is here: http://www.opensource.apple.com/source/ncurses/ncurses-31/include/term.h. I think term.h is a bit of a mess, just like the curses.h from PDCurses... both of them #define too much stuff and some of the #defines are poorly named and likely to conflict with other code, in both cases... this is especially true for lowercase #defines that have short names. That kind of programming practice is highly discouraged nowadays, and in fact, often called "evil": http://hbfs.wordpress.com/2009/11/17/defines-are-evil/ Of course these are the headers for the libraries included in the project, we can't exactly change the fact that they use evil programming practices. So yea, term.h defines "newline" as "CUR Strings[103]", and prior to this defines "CUR" as "cur_term->type.", so this leads the compiler to evaluate "newline" as "cur_term->type.CUR Strings[103]", based on the text of term.h. Instead of calling it "newline" they should've called it "_NEWLINE". They have some equally egregious #defines, such as defining "lines", "columns", "buttons", "bell", "tab", "tone", and "pulse", all of which are short lowercase words very likely to be used as names of variables or functions and lead to conflicts. So while I am responsible for this problem occurring, I think the shoddy practices for naming the #defines used in term.h are really what's to blame even more than me... they should've either used all-caps or preceded with underscores, preferably both. Even better would be if they had thought up a way to do it that didn't involve hundreds of #defines in the first place, and made their code object-oriented rather than exposing so many internals to the public namespace. OK, so maybe that last bit about object-oriented and avoiding #defines wasn't really possible since the header is also supposed to work in vanilla C as well, not just in C++... but my point about the names of the #defines still stands. When I look at that header and look at a MinGW header, the MinGW header is about a billion times better in terms of following recommended C/C++ programming practices and not defining stuff that would be likely to override variable or function names. PDCurses is guilty of the same thing in its header, but not to such an extent, as it mostly uses uppercase #defines.

[Jonathan S. Fox]

I could move term.h's include back into common.h with the rest of the includes of external libraries if I put in the #undef for newline

Please don't do this; I would have suggested it if I felt it was better than moving the include out of common.h. There is no reason to include term.h in the rest of the project, and good reason not to. As you have observed, it's a very 'dirty' include with regard to the global namespace, in that it defines a large number of macros that use common words in all lowercase. Including it with the rest of the project is something to be actively avoided rather than achieved, since it's an invitation for future cryptic linux-specific bugs.

This is an extreme case that demonstrates the value of not "over-including". Normally, a C++ project will have a single header file for each cpp file, where the header exposes the functionality implemented in the cpp file. The cpp file then includes its own header file and any header files corresponding to other parts of the project it depends on. You generally don't have a single include that puts the entire project and all of its dependencies into the namespace all at once; you take what you need on a file-by-file basis. Need characters? Include characters.h. Need politics? Include politics.h. Need combat? Include combat.h.

At best, what LCS does is force a recompile of the entire project every time any header modification is made, which slows down compile times substantially. At worst, you get cryptic problems like the term.h one, where the namespace gets filled up with so much extraneous cruft that unexpected side effects start to occur.

In the case of the complex preprocessor logic to support multiple platforms and three versions of curses importing, I grok what you're saying about the benefits of keeping that logic in one place. Just be careful not to take it to extremes; there are virtues of segmenting your imports too. Because of the messy defines in term.h, it especially should be kept as limited as possible.

[Liberal Elitist]

Don't worry, I was just pointing out how stupid it'd be to include term.h globally, in that last post, I think I explained quite well why it doesn't belong there... as did you... it has way too many defines that are short lowercase names and would mess up the global namespace, plus since it's only included on POSIX systems, Windows programmers could write functions or variables that'd use words defined in term.h and it'd compile and build fine on Windows but totally screw things up on POSIX systems. In fact I think I gave a fairly long rant about why it was unsuitable for including in the global namespace. It's rather funny to me that you responded to my rant about why something is a bad idea by telling me to please not do it. Yes, I know not to do it. I just explained why nobody should do it... I totally agree with you about that. Kind of a funny misunderstanding. I obviously never would have moved term.h's include into common.h in the first place if I had any idea about all the defines in term.h. It was a mistake due to me not knowing the source code of that file since it's only present on POSIX systems. When I was saying I "could" put it back there, that was just part of a longer rant explaining why it was such a terribly bad idea.

[SuicideJunkie]

When I was searching the files, I didn't see very many uses of the newline function at all.
Perhaps it would be a good idea to rename the log function to something else instead?

[Liberal Elitist]

When I was searching the files, I didn't see very many uses of the newline function at all.
Perhaps it would be a good idea to rename the log function to something else instead?

If you want to go ahead but it seems unnecessary to me. It's good, though, for functions to have relatively short, human-readable names that tell what they do, and I think "newline" is a good name for a function that creates a new line. Maybe there are other names that have equally obvious meaning. So yeah if you want to do that go ahead but I think it's unnecessary. If you do change it, you could maybe just change it to "new_line" with an underscore, or something like that. But honestly, this isn't a problem unless consolesupport.cpp needs to use the logging functions. Then again, it's possible someday we might want to use logging functions in consolesupport.cpp, and need to call the newline() function, and in that case it WOULD need to be changed. But that seems unlikely to me... if that problem DOES happen we can just change it then.

Oh yeah, and, since you obviously use a POSIX-compliant operating system instead of Windows, could you tell me, does my fix in revision 823, to fix display of extended characters, actually work on Linux? I am hoping that I have fixed it for you and others who use POSIX-compliant operating systems. I got all 3 code page modes working on Windows: Code Page 437, the ASCII hack, and Unicode. I was able to compile and build it and have everything display correctly in all 3 of those modes after the changes I did in revision 823. I think the changes I made are cross-platform, and thus will also fix the ASCII hack and Unicode on POSIX-compliant systems like Linux and Mac OS X too, not just on Windows. But it would be helpful if someone verified that this was fixed for me. I don't have access to a proper system for testing that in, so somebody else needs to confirm whether or not the problem is fixed.

[Jonathan S. Fox]

I wouldn't rename the newline() function. It's a good name for the reasons Liberal Elitist mentioned, and in the event that we need the logging functions in consolesupport.h, we can also just undefine the newline macro like you did the first time. I strongly doubt we'll ever be using it.