Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | ||
---|---|---|
23 | Can we give a more precise type for this field? It only seems to take on ELF::STT enum values. |
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | ||
---|---|---|
23 | in the ELF , the Type is means symbol type(in all the info in the SymbolInfoTy should be symbol related. ) , it is only ELF object file use the data member now. We will use the Type data member to store "Store Mapping class" of symbol in next patch, it is a refactor patch , I prefer to keep the Name "Type" here, we can change the name to other in the next patch. |
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | ||
---|---|---|
23 | Sorry, to clarify I'm proposing changing the type of the Type field, not the name (it's confusing to talk about the type of a field named Type unfortunately). Specifically I mean naming th ELF unnamed enum involved in BinaryFormat/ELF.h and specifying it here (an enum class would be nice but that may not be possible). Something like: struct SymbolInfoTy { uint64_t Addr; StringRef Name; Elf::STType Type; } Provided the enum was named to STType. We would then introduce a union in the follow on patch. |
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | ||
---|---|---|
23 | change to type Elf::STTType and I need to added a new include header file BinaryFormat/ELF.h and the Type "Elf::STType" will be change soon to "int16_t" in later patch. |
LGTM (with note about later follow on)
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | ||
---|---|---|
23 | Summary of offline discussion: This would probably necessitate revamping the enums in elf.h quite a bit, so this probably is best done as a latter follow on patch. |
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | ||
---|---|---|
25 | Any reason why a constructor is needed? It could be an aggregate and initialize with {}? |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
---|---|---|
344 | Prefer this to be a friend function inside of struct SymbolInfoTy, as this operator could be used in other compilation unit. Also, the function would be easier to reason about and maintain if we write it as: return std::tie(P1.Addr, P1.Name, P1.Type) < std::tie(P2.Addr, P2.Name, P2.Type); } |
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | ||
---|---|---|
12 | Since you add the header of StringRef, then forward declaration of class StringRef can be removed on line 33? | |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
345 | Based on Jason's comment, maybe you can further simplify the function as: bool operator<(const SymbolInfoTy& ST) const { return std::tie(Addr, Name, Type) < std::tie(ST.Addr, ST.Name, ST.Type); } |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
---|---|---|
345 | Re @Xiangling_L |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
---|---|---|
345 | Another article talks about overloading operator function, hopefully it would help: https://www.learncpp.com/cpp-tutorial/94-overloading-operators-using-member-functions/ |
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | ||
---|---|---|
12 | good finding . I will delete is in another patch | |
25 | yes, I think we need it in the code Symbols.insert( Symbols.begin(), SymbolInfoTy(SectionAddr, SectionName, Section.isText() ? ELF::STT_FUNC : ELF::STT_OBJECT)) | |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
345 |
|
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
---|---|---|
345 | Thank you for providing those articles, they are really helpful. And I agree that there are extra benefits comparing hidden friend function than member function in this case. |
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | ||
---|---|---|
25 | Okk. This line does not have proper indentation I think. If we are going to fix other issues mentioned below, I would prefer to have it fix as well. | |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
345 |
|
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
---|---|---|
345 |
Also, since this is an NFC patch, we are not suppose to change any functional behavior right? |
Actually, disregard my last comment. I think I might've made a mistake on the bisect. Sorry!
Since you add the header of StringRef, then forward declaration of class StringRef can be removed on line 33?