Follow up remark: One thing that's also somewhat relevant is the following file in whiteboard:
https://github.com/wesnoth/wesnoth/blob/master/src/whiteboard/typedefs.hpp Most of the whiteboard compilation units include this file instead of any of the boost pointers that they make extensive use of, so you might think this would not be allowed in a "strict include-what-you-use" policy. But, I think that's actually not the right way to understand it. It's more like, the exact choice of smart pointer to use for the whiteboard was not totally decided / left as an open question. In the event that we move to C++11 for example, it's very easy to change over all the pointer types now, you only need to change that file. Only the typedefs are actually used in the compilation units, they don't actually use the boost::shared_ptr type, so they don't need to include boost/shared_ptr themselves. On the other hand, if for example some distant file in gui or ai were for some reason including src/whiteboard/typedefs.hpp, and therefore saying "okay, now I don't need to include boost/shared_ptr", that would clearly be bad news for someone who makes changes to the whiteboard. typedefs.hpp should not be used as a source for the boost::shared_ptr type, there is only one correct source for that (in a strict policy). typedefs.hpp is a correct source just for the whiteboard typedefs that it defines. Anyways if you are looking for ways to cut down on number of explicit boost / STL includes used in some module, a file like that is probably not a bad way to do it, IMHO, and it will make it easier to change over the types of containers / pointers you are using if you later decide that's necessary for some reason. Best Regards, Chris Beck On Tue, Jul 8, 2014 at 10:54 PM, chris beck <beck...@gmail.com> wrote: > Okay, sorry for the change of tone, I think I didn't fully understand > where you were coming from the first time / what the scope of the > discussion was. > > > "iostream is included by log.hpp. ..." > > As mentioned, that style of arranging headers is, afaik, generally > considered bad. Your compilation unit should not make assumptions about > what includes you are getting from other headers, it should directly > include every definition you need to make it work. Otherwise, if someone > starts (1) cleaning up unnecessary includes (2) refactoring how some part > of the code works, your compilation unit will break and you will force them > to clean up. Why is it a reasonable assumption that <iostream> will always > be included by log.hpp? What's to stop it from being forward declared > instead in the future, or that the symbol wouldn't be needed in log.hpp at > all? Should those things happen, wesnoth.cpp will break for no good reason. > It doesn't actually need iostream by necessity of interacting the logger, > it needs it for many other things as well. So it should include it > separately. > > As I understand this is not a new principle, it's also the same reason why > * you should not dereference pointers you got from some distant part of > the code without checking if they are null. For example one of the > resources.hpp links. Why? Because even if you are 100% sure that at the > current revision they will never be null when you dereference, what if some > other part of the code changes and now something can be null that wasn't > before? Then you have undefined behavior, and no one working on the other > part of the project could reasonably have known that that would happen. > * you should not throw exceptions out of a destructor. We talked about > this in an earlier thread, and actually there are many reasons that this is > bad. But one of them is, if your destructor is called while another > exception is being processed and you throw, then you just crashed the > program. And it is a very bad idea to assume that there will never be > another exception on the stack when your object is being destroyed, because > we make extensive use of long range exceptions, and someone else might > throw another one into the mix very easily. And there's no reasonable way > for you to check whether there is or isn't an exception on the stack. You > shouldn't bake into your module extensive assumptions about exactly how > distant parts of the code are arranged. > > Code should always make self-contained assumptions to the extent possible. > When you don't do this, the result is fragile code which produces cascading > bugs when a change happens or any other bug happens. > > I have never known anyone to advocate getting your definitions through > chains of headers. When it happens it's generally a result of accidents, > and I have a hard time seeing how to have a serious policy debate about it, > because of how much less maintainable the code base becomes, how much > harder it becomes to cleanup headers and to refactor classes. What possible > benefits are there to doing this intentionally, besides perceived aesthetic > benefits? > > > I suspect our current policy either differs from this or leaves this > point > > unspecified, otherwise this discussion wouldn't be taking place, and the > > commits in question wouldn't have happened. > > I don't see this (about how under some circumstance, the commits in > question wouldn't have happened). Unfortunately, all programmers make > mistakes with includes periodically, and because these kinds of mistakes do > not generate compile or run time errors, they stay that way indefinitely > until a compilation unit breaks unexpectedly, or compilation times go to > hell and someone takes the time to fix all the headers. Periodic header > cleanup is necessary in all projects. There are a few ambitious tools like > deheader (which tests to see which headers can be removed which breaking > compilation) and iwyu (which actually runs the clang lexer and parser and > checks which symbols require inclusion / can be forward declared) that > attempt to automate the process but none of them quite work perfectly. You > should expect to find mistakes like these in any project, and I have never > encountered anyone who considers it bad to fix them. > > I think the most likely thing is that people simply weren't conscientious > about the headers when they are editing multiple files, it's understandably > difficult sometimes to keep track of which things are in scope in which > file. > > Here's an example commit, where I removed a totally unnecessary > "resources.hpp" include from unit.hpp, and I was forced to fixup nearly a > dozen files as a result. A similar thing happened iirc when I removed > "formula_string_utils.hpp" from either team.hpp or unit.hpp (can't remember > now) > > > https://github.com/wesnoth/wesnoth/commit/1796b4d7a0e6d712b092a0785ea437ad816e7165 > > Do you really think that people were thinking to themselves, "hmm I know I > need to include the definition of resources now somehow, now that I am > using it for the first time in this file, but it's okay since I know I'm > getting that from unit.hpp"? I don't think there's any real possibility > that that was what was happening, rather someone is just working away > trying to fix a bug, perhaps with only so much time to allot to the task, > they realize what code expression will fix it, then they type that in, > compile, test and commit. I really don't think anybody is coding away > thinking to themselves "okay, I've got my resources.hpp from unit.hpp, and > my formula_string_utils.hpp from team.hpp, check and check..." > > Still, I guess you seem to suggest otherwise though, that perhaps some > people were doing this intentionally, knowing that they get <iostream> from > "log.hpp" etc.? Myself, I would much rather not try to memorize what > includes are in every header file, and instead just include all the > definitions I use and rely on the include guards. All modern compilers use > "include guard optimization", so that running into an include guard > repeatedly does not even trigger a new file read and is essentially free at > compile time. > > > Along the same lines, I'd like to know if it's intended to become a > formal > > policy of ours to add comment lines specifying the rationale for every > single > > trivial inclusion, or whether this is just a temporary or accidental > thing > > that wasn't supposed to be included in the commits. > > I don't see any reason why it should be either mandated or prohibited, or > even mentioned in the coding standards. Code comments that explain things > are usually considered good, I don't see why these would be considered > harmful. The vast majority of those were generated automatically but some > of them I added manually after inclusion fixups. If they bother you, it > would be quite easy to delete them all with a sed command, or to put them > back later. > > > The reason I want to know all this is that I intend to continue > contributing > > *new* code in the future (and I'm working on this right now, in fact), > so I > > would greatly prefer to adjust my coding style in the present rather than > > having to go back to clean up after the fact later. I'm sure this is not > an > > unreasonable position. > > Okay, to the best of my knowledge, the best practices are: > > Include all the definitions from your project that you need. > Don't include any files that you don't need. > Use forward declares when you can get away with it. It's not an "error" > not to do this, but it is a missed opportunity. > Follow the guidelines for the library -- oftentimes they have headers that > are not intended to be included, and you need to include a higher up file > instead. For instance don't include any boost file with "detail" in it's > path or name. For STL you generally should include all definitions directly > -- see for example here: > http://stackoverflow.com/questions/2788388/when-is-include-new-library-required-in-c > > If something is wrong, we can generally spot it and fix it later, and it > it doesn't compile for someone they will let us know and we can fix it. > Having messed up includes is annoying but its not at actually at all in the > same ballpark as getting undefined behavior. If you are intending to make > patches I dont think you should be worried that our policies about includes > have somehow changed or something, it seems that they have always been > simply unstated / use your judgment. If you think it's important that we > make a clarifying remark on the wiki of some kind, I don't see any reason > against it but at the same time I think it's not nearly so important as > other guidelines we have like "write exception safe code". Also, keep in > mind that we also don't put in "common sense" things about C++ like "use > include guards", although afaict that is pretty much mandatory in this > project. > > There's a pretty expansive list of the possible benefits of "a strict > include what you use" policy listed here: > http://code.google.com/p/include-what-you-use/wiki/WhyIWYU > > It's good reading but for many reasons a "strict" policy it's not at all > practical. However, when reading keep in mind that what they are talking > about is "strict iwyu" meaning e.g. running iwyu for fixups after every > single commit. Afaik what all actual, even large, projects do is more like > "loose iwyu" meaning "more or less manually fixing up the code to include > exactly what is necessary, where possible and whenever someone decides to > do some header cleanup". > > Best Regards, > Chris Beck > > > On Tue, Jul 8, 2014 at 7:31 PM, Ignacio R. Morelle <shadowm2...@gmail.com> > wrote: > >> On Tuesday 08 July 2014 18:17:42 chris beck wrote: >> > Ignacio: >> > >> > I looked again at the includes in wesnoth.cpp, I'm not sure I understand >> > >> > what you are referring to here: >> > > especially those coming from libraries such as SDL and the STL >> > > (which have become even more redundant) >> > >> > I believe that all of those STL inclusions are necessary. >> >> iostream is included by log.hpp. SDL.h includes SDL_error.h, SDL_events.h, >> SDL_stdinc.h (via SDL_main.h), SDL_timer.h, SDL_version.h. Pretty much >> every >> header of our own included brings string. config.hpp brings tstring.hpp, >> ctime, map, utility, and vector, although you could argue that only >> tstring.hpp and vector are guaranteed to come from there since they are an >> essential requisite of the config interface. Et cetera. >> >> > But regardless I'm also not sure I understand the premise of this >> thread -- >> > why should we have special policies for includes beyond "use best >> practices >> > and follow the standard guidelines for portably including libraries?" >> > (presumably our current policy??) >> >> I suspect our current policy either differs from this or leaves this point >> unspecified, otherwise this discussion wouldn't be taking place, and the >> commits in question wouldn't have happened. >> >> If you check the current version of CodingStandards in the wiki [1], you >> will >> not find a single mention of "inclusion", "include", or "header". >> >> 1: http://wiki.wesnoth.org/CodingStandards >> >> It's true that in the past we've had countless issues with platform >> differences and STL and C includes, which could have been avoided with a >> more >> fine-grained approach like this. However, to my knowledge none of them >> have >> been particularly catastrophic, and because most of us aren't professional >> coders with a vast knowledge of cross-platform support intricacies I'd >> expect >> the problem to repeat itself in the future unless we are specifically >> reminded >> at every opportunity to add include lines for every single thing we use. >> >> Along the same lines, I'd like to know if it's intended to become a formal >> policy of ours to add comment lines specifying the rationale for every >> single >> trivial inclusion, or whether this is just a temporary or accidental thing >> that wasn't supposed to be included in the commits. >> >> The reason I want to know all this is that I intend to continue >> contributing >> *new* code in the future (and I'm working on this right now, in fact), so >> I >> would greatly prefer to adjust my coding style in the present rather than >> having to go back to clean up after the fact later. I'm sure this is not >> an >> unreasonable position. >> >> > In general I hope that when you see an unnecessary include, you will (1) >> > simply fix it, (2) if it is systemic, double-check the guideline and >> then >> > PM the person / people who are making the mistake. If it is your >> intention >> > to send an email to the dev list assigning blame every time you see an >> > unnecessary include, I hope you will give us some advance warning so >> that >> > we can change our email filter rules, as this could lead to a ton of >> > email... I also don't think it's realistic to expect that writing e.g. >> > "don't make unnecessary includes" on the wiki is going to magically >> prevent >> > the situation from repeating itself. Getting the includes right is >> > unfortunately a hard problem that all projects struggle with. >> >> I chose the mailing list as the venue for this discussion for several >> reasons: >> >> 1. We only superficially broached the subject on IRC the other day. In >> general, I do not consider IRC to be a suitable medium for long >> discussions >> covering topics of permanent relevance like this, and this isn't the first >> time we've talked about coding conventions in this mailing list. >> >> 2. IRC is a volatile medium. If you don't specifically schedule a meeting >> with everyone beforehand, it's extremely unlikely that people reading the >> logs >> later will want to resurrect a previous topic of discussion unless it >> concerns >> them directly. For touchy subjects like this, I also find it more >> appropriate >> to obtain opinions from as many other people as possible in the hope that >> I >> can learn something instead of perpetuating an echo chamber situation. >> >> I'm slightly unsettled by the sudden change of tone from your previous >> email, >> especially given that it seems to rest on a questionable assumption about >> both >> my future usage of the mailing list and my willingness to use an email >> client >> in the first place. However, I reckon it's my fault for not choosing my >> words >> in the opening message more carefully, so I apologize if I sounded >> tactless >> about a subject that concerns us all. >> >> And going back to that previous email: >> >> On Tuesday 08 July 2014 00:16:26 chris beck wrote: >> > I think the problem is not the number of inclusions, rather the variety >> of >> > code used in that file. (Or rather, the number of includes is a symptom >> > rather than the cause.) >> >> True, there is still some stuff in there that might make sense to move to >> other files, although the current situation is comparatively better than >> it >> used to be. >> >> $ wc -l wesnoth-1.[468]/src/game.cpp wesnoth-1.1[02]/src/game.cpp \ >> wesnoth/src/wesnoth.cpp | head -n -1 >> 2371 wesnoth-1.4/src/game.cpp >> 2266 wesnoth-1.6/src/game.cpp >> 2123 wesnoth-1.8/src/game.cpp >> 680 wesnoth-1.10/src/game.cpp >> 797 wesnoth-1.12/src/game.cpp >> 878 wesnoth/src/wesnoth.cpp >> >> > If someone makes a change to a faraway header file somewhere in the >> project, >> > it should not break *this* compilation unit. Otherwise you end up >> making a >> > lot of work for other people -- if someone tries to remove an >> unnecessary >> > include somewhere, it breaks more than a dozen distant unrelated >> compilation >> > units. >> >> This is true, but see the config.hpp and tstring.hpp + vector case above >> for >> an example where (as far as I can tell) this shouldn't happen unless the >> interface published by the header changed completely -- in which case it >> should be the programmer's responsibility to update all client code >> accordingly, since this is all part of a monolithic project and not a >> shared >> library. >> >> > If you think think that wesnoth.cpp has too many includes, the best >> thing >> > would likely be if you split it into several compilation units. A glance >> > suggests that there are many things in that file that don't need to be >> > together. There are many files in the project that would be a bit >> better to >> > split up, it's only one example. There are surely still unnecessary >> > includes in various places but if you are mainly interested to fix up >> > "unsightliness" that seems to be the obvious solution. >> > >> > I have done much work lately to split up files that are too big / >> bloated, >> > any assistance in this regard would be greatly appreciated. >> >> I personally did this in 1.5.x before with the add-ons management UI >> code, all >> of which was originally crammed into game.cpp along with the (long since >> gone) >> GUI1 campaigns selection dialog, command line options processing, and the >> game >> (not play) controller stuff. It's unlikely I can help with this endeavor >> at >> the moment, because my current focus for the past two series has been >> working >> on front-end usability and UX issues, and in order to refactor code I >> need to >> fully understand the underlying logic and the functionality it implements, >> which I don't for the remainder of game.cpp I haven't ever touched (sans >> the >> few exceptions where I've been forced to deal with it to address the >> aforementioned issues). >> >> However, this invitation ought to be useful for the other developers here >> on >> this mailing list, which is all in accordance with this thread's intent. >> >> -- >> Regards >> Ignacio R. Morelle <shadowm> >> >> _______________________________________________ >> Wesnoth-dev mailing list >> Wesnoth-dev@gna.org >> https://mail.gna.org/listinfo/wesnoth-dev >> > >
_______________________________________________ Wesnoth-dev mailing list Wesnoth-dev@gna.org https://mail.gna.org/listinfo/wesnoth-dev
