Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
tools/yaml2obj/yaml2elf.cpp | ||
---|---|---|
229 ↗ | (On Diff #176402) | "check" implies to me that it's just validating something. However, this function does more than that (it sets the Index variable too). I'd suggest renaming the function convertSectionIndex or similar. |
230 ↗ | (On Diff #176402) | SecField to me implies the name of a section field, e.g. "sh_link". I'd suggest renaming SecField to IndexSrc or similar, and Index to IndexDest. The two are closely related, so having similar names makes sense to me. |
278 ↗ | (On Diff #176402) | This isn't a section index lookup. It's a symbol lookup. Note that the message is different. |
tools/yaml2obj/yaml2elf.cpp | ||
---|---|---|
278 ↗ | (On Diff #176402) | By the way, were there no test failures because of this? If there were none, I'd suggest adding a test for this particular case, again in a separate change. |
tools/yaml2obj/yaml2elf.cpp | ||
---|---|---|
278 ↗ | (On Diff #176402) | Sure, I'll add one. |
tools/yaml2obj/yaml2elf.cpp | ||
---|---|---|
278 ↗ | (On Diff #176402) | I noticed this problem, because they are all of type NameToIdxMap. |
tools/yaml2obj/yaml2elf.cpp | ||
---|---|---|
239 ↗ | (On Diff #176583) | As this is only used the once, I don't think that it is useful to pull this case out into a separate function. I could be persuaded otherwise though. |
I just want to make the logic inside initSectionHeaders simple. I could wait until there are two places that needs simplify ...
Thank you @jhenderson, for your kind reviewing :)