Added XrefsDBManager into include-fixer and made XrefsDB return SymbolInfo.
Details
- Reviewers
klimek djasper hokein - Commits
- rG692aca698090: Added XrefsDBManager into include-fixer and made XrefsDB return SymbolInfo.
rCTE268480: Added XrefsDBManager into include-fixer and made XrefsDB return SymbolInfo.
rL268480: Added XrefsDBManager into include-fixer and made XrefsDB return SymbolInfo.
Diff Detail
Event Timeline
include-fixer/InMemoryXrefsDB.cpp | ||
---|---|---|
18 | I'd make that a const ref. | |
24–26 | I assume it's intentional that SymbolInfo doesn't have a constructor; what's the reasoning behind that? | |
include-fixer/IncludeFixer.h | ||
34–35 | Why wouldn't we still use the interface here? (usually we want to take the least specific type we can deal with) |
- Addressed reviewer comment.
include-fixer/InMemoryXrefsDB.cpp | ||
---|---|---|
24–26 | SymbolInfo can be of different kinds, e.g. Function, Class etc. and has optional fields. I think this was the reason we didn't have a constructor for it. @hokein, right? | |
include-fixer/IncludeFixer.h | ||
34–35 | XrefsDBManager now provides a different interface. XrefsDB provides std::vector<SymbolInfo> search(...), while XrefsDBManager provides std::vector<std::string> search(...), which just returns the #include paths. |
include-fixer/IncludeFixer.h | ||
---|---|---|
34–35 | That seems unexpected. Why is the XrefsDBManager in the business of doing SymbolInfo -> include path mappings? |
include-fixer/IncludeFixer.h | ||
---|---|---|
34–35 | Since the mapping was done in XrefsDB before, I kept the way it was. Migrating SymbolInfo -> Include path mappings into IncludeFixer will be the next step, and I'll need to discuss with Ben about where exactly we want to place it. |
include-fixer/InMemoryXrefsDB.cpp | ||
---|---|---|
24–26 | Yeah, the intention is that it's not easy to construct a completed instance at one time since it has many fields. |
lg
include-fixer/InMemoryXrefsDB.cpp | ||
---|---|---|
24–26 | Overall, I think creating an instance and then setting members should only be done from places very strongly coupled to the class. (also, specifically regarding the comment that it's not easy to construct a completed instance: that is a large hint that it should be encoded somewhere close to the class) | |
include-fixer/IncludeFixer.h | ||
34–35 | Please add a FIXME that this is going to change. | |
unittests/include-fixer/IncludeFixerTest.cpp | ||
86 | identifiers |
I'd make that a const ref.