> On Oct. 6, 2016, 3:57 p.m., Jason Lowe-Power wrote: > > Seems reasonable to me. Could you explain in the commit message how you > > decided which headers to remove/keep/add? Without digging into these files > > it's hard to understand your thought process. > > > > Related: Do you think it makes sense to codify which headers to include in > > the style guideline? IMO, this would be helpful so you don't have to go > > through and do this again :). > > > > One other small thing. I don't think we should hold up this patch for this > > issue, but it's something that's been bothering me. It seems the style > > checker is complaining about the ordering of includes for this patch, > > though I don't think they are actually wrong. When I run the following I > > get errors: > > > > ``` > > hg qref COPYING # to get the patch to be "unapplied" but the changes > > reflected to the files > > util/style.py > > ... lots of errors. For every single file it complains about the ordering > > of includes. > > ``` > > > > If I just use hg qref, the style checker doesn't complain. Ideas? > > Brandon Potter wrote: > I think that the style checker is buggy. It's kind of beautiful (or maybe > completely disgusting), but my current patch queue has a patch that refuses > to ever properly resolve an "hg qref". I run "qref" once and it resolves with > a single issue about wanting to fix a line. If I try to run it again without > making any changes, it decides that it's still not happy and wants to make > more changes. This continues on ad nauseum. So yeah, beauty. > > Regarding your comment about general header guidelines, developers should > try to do a better job about checking what their includes actually include. I > don't think this is enforceable just with my complaint; we need to cook this > into the check-in process if we want it to stick. > > I've been aware that the include problem has been around for a while, but > I've largely ignored it due to lack of motivation about tackling a problem > that no one probably cares about (and would probably just cause me the > headache or coding it up to have it rejected). However, I ran into an issue > where I added a new dependency between headers files recently. It was kind of > interesting to find out that it involved a circular depenendcy between header > files between 4-5 headers. Each header had included another header and > eventually created a chain. (I think that it was a bit more convoluted in > reality and I may not have even understood it correctly, but there's a > definite problem here.) > > My first pass, that you reviewed, was done manually. I opened header > files (and their included header files) and tried to figure out where things > are coming from. I focused mainly on the "sim" directory, because that's > where most of my work has been done. I posted the results after resolving > some of the problems and that's what you have now. > > While working on this problem, I figured that this is probably a more > general problem for any large project. I Googled around and found a tool that > can give recommendations on header dependencies: cppclean. I've been using > the tool to try to resolve some of the issues mainly focusing on using > forward declarations where possible. It's pretty time consuming given that I > need this to work for all of the ISAs and their Ruby protocols. I've gone > through "src/sim" completely now with the tool and have resolved everything > that wasn't a false positive; the tool doesn't handle macros very well (and > our build process is wonky given that it uses swig). __It's really time > consuming; I don't know that I'm going to pursue it any further unless people > feel that it's worthwhile.__ I'm under constraints here at AMD to post > unrelated patches so I doubt that I'll have much support in my own corner to > spend much time on this. I'm mainly looking at ARM and Wisconsin folks to see > what their opinions are regarding the headers. If it's a widespread problem > that people want fixed, I can try to see if I can pursue this for the > remaining directories in "src". > > Anyways, the patch is going through regressions now. I'll post the update > when it finishes.
I think I know what's going on here. I suspect that the include order checker doesn't get enough context in one of the cases and doesn't spot blocks with incorrect include order. In particular, the primary header first rule (i.e., foo.cc should include foo.hh first) tends to get confused. The solution to this is likely to always check the entire file in this particular checker. We probably want to apply the sorted include checker to all of the files in the source tree first though. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3643/#review8772 ----------------------------------------------------------- On Oct. 5, 2016, 5:27 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3643/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2016, 5:27 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11660:1c76ab8bafec > --------------------------- > style: reduce include dependencies in some headers > > > Diffs > ----- > > src/arch/x86/system.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/x86/tlb.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/x86/tlb.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/x86/utility.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/x86/utility.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/kern/linux/linux.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/kern/linux/linux.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/mem/multi_level_page_table.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/mem/multi_level_page_table_impl.hh > b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/mem/page_table.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/mem/page_table.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/sim/arguments.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/sim/process.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/sim/process.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/sim/sim_object.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/sim/sim_object.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/sim/syscall_emul.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/sim/syscall_emul.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/generic/tlb.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/power/interrupts.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/x86/pagetable.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/x86/pagetable.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/x86/pseudo_inst.cc b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > src/arch/x86/system.hh b29aca3fcb75f5ad92429001ab11c65b2f9635b0 > > Diff: http://reviews.gem5.org/r/3643/diff/ > > > Testing > ------- > > util/regress > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
