This is an archive of the discontinued LLVM Phabricator instance.

Added static creators that create complete instances of SymbolInfo.
AbandonedPublic

Authored by ioeric on May 4 2016, 4:51 AM.

Details

Reviewers
klimek
hokein
Summary

Added static creators that create complete instances of SymbolInfo.

Diff Detail

Event Timeline

ioeric updated this revision to Diff 56127.May 4 2016, 4:51 AM
ioeric retitled this revision from to Added static creators that create complete instances of SymbolInfo..
ioeric updated this object.
ioeric added reviewers: klimek, hokein.
ioeric added a subscriber: cfe-commits.
klimek added inline comments.May 4 2016, 5:29 AM
include-fixer/find-all-symbols/SymbolInfo.h
98–101

To some degree this looks like we actually want a class hierarchy. But currently only ClassSymbolInfo is used?

hokein edited edge metadata.May 4 2016, 5:37 AM

Not sure whether this is what @klimek expected..

You also need to update the FindAllSymbolsTests code (there is a CreateSymbolInfo function there).

include-fixer/find-all-symbols/SymbolInfo.cpp
91

code indentation.

105

A extra blank line.

115

The same.

include-fixer/find-all-symbols/SymbolInfo.h
99

Use llvm::StringRef instead of std::string&.

ioeric added inline comments.May 4 2016, 5:38 AM
include-fixer/find-all-symbols/SymbolInfo.h
98–101

At this point yes, since Symbolnfo is only created in InMemoryXrefsDB and FindAllSymbols now, and it is not trivial to change FindAllSymbols to use these creators.

But one immediate use case I can foresee is converting proto buffer SymbolInfo to clang SymbolInfo when integrating with Google3 XrefsDB.

klimek added inline comments.May 4 2016, 5:56 AM
include-fixer/find-all-symbols/SymbolInfo.h
98–101

I think at this point YAGNI applies and we shouldn't create stuff that we don't use yet - code generally starts to rot when it's not used ;)

ioeric updated this revision to Diff 56139.May 4 2016, 6:49 AM
ioeric marked 7 inline comments as done.
ioeric edited edge metadata.

Use static creator functions in SymbolInfo in FindAllSymbolTests, and make SymbolInfo::operator== compare all fields.

ioeric updated this revision to Diff 56143.May 4 2016, 6:57 AM
  • Use static creator functions in SymbolInfo in FindAllSymbolTests, and make SymbolInfo::operator== compare all fields.
ioeric updated this revision to Diff 56145.May 4 2016, 7:07 AM
  • Use template to compare llvm::Optional<T> types.
hokein added inline comments.May 4 2016, 7:13 AM
unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
91–92

You can remove it now.

ioeric updated this revision to Diff 56147.May 4 2016, 7:16 AM
  • Removed unused function in unit test.
ioeric updated this revision to Diff 56148.May 4 2016, 7:33 AM
  • Removed SetCommonInfo declaration from header.
hokein added a comment.May 4 2016, 7:40 AM

It looks good to me now, but need to wait @klimek acceptance.

include-fixer/find-all-symbols/SymbolInfo.h
98–101

However, changing SymbolInfo to class requires us to add many setters/getters in it.

Currently the SymbolInfo is like ClangTidyOptions structure.

klimek added inline comments.May 4 2016, 8:51 AM
include-fixer/find-all-symbols/SymbolInfo.h
98–101

So, multiple thoughts:

  • generally, I think symbol info would better be an immutable type; that would mean that we wouldn't have setters, just getters, and initialize everything in the constructor
  • afaik llvm/clang style doesn't require accessors just because something is a class
  • I believe ClangTidyOptions is a very different use case
ioeric added inline comments.May 9 2016, 2:26 AM
include-fixer/find-all-symbols/SymbolInfo.h
98–101

But this change would also affect the way SymbolInfo is constructed in FindAllSymbol - class members are retrieved from AST in different locations, and we'd need to store fields in many temp variables, which might be troublesome in this case as well as some other cases. With a struct implementation plus static creatrors, we can have some flexibility. But I'm not sure if this makes sense from design perspective.

ioeric abandoned this revision.May 12 2016, 8:40 AM