2.0.x dependency spaghetti #8497
Comments
thinkyhead
added
T: Design Concept
|
There must be tools that generate dependency graphs. And assuming such a tool exists, it probably will tell us how many declarations are in each header file that are needed by the .cpp file. At: https://stackoverflow.com/questions/42308/tool-to-track-include-dependencies they claim -M -H options on gcc produce a dependency tree. |
Apparently "If you have access to GCC/G++, then the -M option will output the dependency list." but "This only works if there are no errors" |
|
Yeah... and supposedly, -H is a tree format of the -M More from that page: For a heavy weight solution, you should check out Doxygen. It scans through your code base and comes up with a website, effectively, that documents your code. One of the many things it shows is include trees. |
|
Aha! Also, Doxygen can make an #include graph (if it can find everything). |
|
Yeah... We are looking at the same web pages! :) |
|
First practical matter to sort out is the If we wanted to sort it out in terms of header dependencies, then… The When originally fashioning So, I'm about to stick |
|
Ah… I see that |
|
Why is Teensy defining a square macro? Maybe it would make sense to pull that definition out of the file. And if they really do need a square macro, can't they use the standard Marlin one? (And in that case... macros.h can be very early in the include order where it belongs.) |
|
It's defined by Teensy's own The solution is straightforward enough. We just have to make sure to redefine |
|
I actually touched a bit on that sq issue when doing LPC SPI work. I just put an That aside, it would definitely be clearer if we could get rid of some of the "collective" header files. Even if that would lead to bigger lists of includes in cpp files. |
@teemuatlut Totally.
Yep! I ended up doing the same thing. Except, re-defining it also.
Agreed. The only time we should have an While tools like Doxygen can tell us what the apparent dependencies are, it can't reveal the real dependencies. For that we'll have to use the compiler. I'm going to try and add the |
|
Okey, to clarify there is one place where I think we definitely should have includes within headers and that's the HAL. Basically keep the idea that a Marlin code file will include the HAL file which then further redirects to the proper implementation and information. |
|
For part of one of my first intern jobs in 2007 or 2008 I had to write a python script that generated a dependency graph and plot it with Graphviz. It's not very hard to do this, and I'd recommend whomever is in charge of this sort of refactoring do it themselves. By the way, for avoiding namespace collisions in macros you might want to look at a system of module prefixes. To keep things from getting too messy, you can have a utility header whose macros are allowed to go prefix-less; everything else must have a module prefix. That way you don't have to deal with the annoyance of spelling out things like MATH_MIN, MATH_FLOOR all the time. For doing the module prefixes, for an example fastio.h macros could be prefixed with FIO_ or FASTIO_, e.g. FIO_GET_INPUT(). |
|
@teemuatlut You can easily end up in a situation where modifying one obscure header forces the whole project to rebuild if you do that. Although I suppose for most people the stupid Arduino build system does that anyway, but eventually you are going to want a proper incremental build. |
|
That's how the HAL system works at the moment (headers into headers). How would you suggest it should be done for future proofing? |
|
Sorry I phrased that wrong. Wasn't supposed to be as assertive as it reads. I prefer to keep my header structure flat, but there's nothing wrong with having a hierarchy of headers so long as you keep the dependency problem in mind. I've done it both ways. |
|
I've applied some preliminary solutions. Most vitally, I split out the first section of #include "../core/boards.h"
#include "../core/macros.h"
#include "Version.h"
#include "../../Configuration.h"
#include "Conditionals_LCD.h"
#include "../../Configuration_adv.h"
#include "Conditionals_adv.h"This file is safer to include from While I'm on the subject of I did a compile of the default config for GCC -M -H Output |
|
The most straightforward way to figure out dependencies is to start commenting out Files that rely on config options, whether |
|
@thinkyhead Do you still need this open? |
|
@Phr3d13 since they are aiming for a RC it might be time to look at it |
Not really. Sorting out the HAL include order is a continuous process and never leaves the TODO list. There are some compiler flags that will produce a report on which files include what, so that will be the primary guide. I would like to standardize the HAL structure and naming conventions too. I think we can trust the compiler to include from the right path, so I don't worry having all files in the HALs share the same names. I'll approach task in tandem with sorting out includes. |
|
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Before going too much further with 2.0.x it seems prudent to work out the interdependency and "hierarchy" of header files. The main issue is that many of the framework, platform, and hardware file headers define their own convenience macros, and many of these are either duplicates of the macros defined for Marlin, or simply use the same name but are defined differently.
I'm not sure how to output a complete "dependency tree", but just doing a search on
#includeshould help to discern the pattern.Each
.cppfile stands alone as its own compilation unit, so each one can include only the headers it needs. Things get a little more complicated when.hfiles include other.hfiles. And things get more complicated still when we need to includeMarlinConfig.h, since this file attempts to smartly enforce a certain order of header includes, which might not be so smart (mea culpa).The issue that sparks this post comes from Teensy 3
wiring.hwhich SPI relies upon.For reference, here's what `MarlinConfig.h` does…
Note that this file includes
HAL/SPI.hearly, but after boards, macros, and Version — none of which it requires. The first question is, why includeHAL/SPI.hhere, since so few of Marlin's.cppfiles require it? Wouldn't it be better to include it only where needed? Perhaps it's because theConfiguration.hfiles refer to theSPI_XXX_SPEEDconstants.The way
#defineworks, this isn't something we need to worry about. As long as the symbols referenced in any#defineare defined before the code that uses them, the pre-processor can figure it out.Consider this example…
test.cpp
test.h
When you run this code, everything is found, even though some things didn't "exist" before they were referenced by other
#definestatements.Keep this in mind when worrying about dependencies. As long as all the referenced symbols are
#included, they can be in any order.So, we don't need to include
HAL/SPI.hinMarlinConfig.hfor a start. And in a similar manner, pertaining to both.hand.cppfiles, we can get rid of#includes where the defined types, methods, etc., are not specifically needed by that file. This will help to prevent name collision. If there are some.cppfiles that need to include bothwiring.handmacros.h, we should make sure to includemacros.h(MarlinConfig.h) only afterward.There are some good rules of thumb to follow when deciding what to
#includeand in what order. This post at Stack Overflow is about right…The text was updated successfully, but these errors were encountered: