Change includes to improve desbordante performance #531
Conversation
- Add includes to the files that are used - Add forward declarations - Remove includes that aren't used or to add their components
PodushkaPIR
force-pushed
the
fix_includes
branch
from
1480ba0 to
439bde1
Compare
There was a problem hiding this comment.
I understand that the pull request is still in draft status. I noticed some small points, they need to be addressed.
At the same time, I want this pull request to help developers and reviewers in the future. I would like to bring up several questions for discussion:
- It would be good to automate this thing if possible. As I see, you used
iwyuto fix the problem with includes. How well does it work, how many manual corrections need to be made, etc.? Will it be possible to add it to the CI? - It would be good to see how this problem is solved in large projects. Probably, it is possible to find projects that can be used as an example in this regard. Do they use this tool, do they add it to CI or any alternative tools?
- It is interesting that we are trying to use forward declarations where possible. I would like to understand how justified this is, since Google Code Style, for example, clearly says to avoid them. If we get a good gain in project compilation time, then that's one thing (have you noticed a reduction in compilation time?). If not, then it's worth thinking about it some more. And in general, it's interesting to learn about best practices regarding this.
- In the end, it would be good to also get some kind of guide for developers (to our wiki), which will describe some of the subtleties regarding our agreements on how we write includes.
| #include "algorithm.h" // for Algorithm | ||
| #include "algorithm_types.h" // for AlgorithmType | ||
| #include "algorithms/create_algorithm.h" | ||
| #include "algorithms/pipelines/typo_miner/typo_miner.h" |
There was a problem hiding this comment.
Most likely we want to use the current style of writing paths (relative to the "src/core").
| #include "create_algorithm.h" // for GetAllDerived | ||
| #include "csv_config_util.h" // for MakeInputTable | ||
| #include "fd/fd.h" // for FD | ||
| #include "gtest/gtest.h" // for Message, Ass... |
There was a problem hiding this comment.
It's interesting that here we see a duplicate include without angle brackets. I don't know why this occurs.
It would be good to check that there are no similar problems?
| {kFdReduced30, 275990379954778425U}, | ||
| {kFlight1k, 2512091017708538662U}, |
There was a problem hiding this comment.
nit: let's change indent here too
| #include "../model/list_agree_set_sample.h" // for ListAgreeSetSample | ||
| #include "../model/pli_cache.h" // for PLICache |
There was a problem hiding this comment.
Since we are fixing includes, it is probably worth fixing things like this.
At least, this file has "fd/pyrocommon/model/agree_set_sample.h" and "../model/list_agree_set_sample.h". We probably want to use paths relative to "src/core/algorithms/" here.
At the same time, there are probably many similar things throughout the project. Шt would be great to fix all of this with some tools.
| #include <chrono> | ||
| #include <memory> | ||
| #include <stdexcept> | ||
| #include <bits/chrono.h> // for duration_cast |
There was a problem hiding this comment.
It looks like we should use <chrono>, isn't it?
About include comments: it's temperary and they don't make sense.