Keep the existing special cases based on combinations of section name, flags and sizes/offsets.
Some of those special cases have been added intentionally, with test cases, rather recently, so I don't want to try to generalize them at the moment.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp | ||
|---|---|---|
| 883–885 | This makes pretty weird control flow. I think it would be way clearer if all of this code were moved into a function like GetSectionType (there's a function like that in ObjectFileELF). Then you can use return statements to shortcut control flow, like so: if (m_sect_headers[idx].flags & llvm::COFF::IMAGE_SCN_CNT_CODE &&
((const_sect_name == g_code_sect_name) ||
(const_sect_name == g_CODE_sect_name)))
return eSectionTypeCode;
if (...)
return eSectionTypeZeroFill;
SectionType type = StringSwitch<SectionType>(name)...;
if (type != Invalid)
return type;
... | |
| lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp | ||
|---|---|---|
| 883–885 | +1. Factoring the decision tree into a separate function is part of what I was trying to ask for in the patch. That it simplifies things with early returns is an added bonus. | |
Comment Actions
Thanks for taking the time to do this. Just get rid of the elses and this is good to go.
| lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp | ||
|---|---|---|
| 803–804 | Now that this is a return, you don't need the else as per http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return. | |
| lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp | ||
|---|---|---|
| 803–804 | Ok. What about the individual if/return/else/return within the if statements? Is it preferred to keep them as is here, e.g. if (condition) {
if (othercondition)
return eSectionTypeZeroFill;
else
return eSectionTypeData;
}Or to skip the inner else in the same way? if (condition) {
if (othercondition)
return eSectionTypeZeroFill;
return eSectionTypeData;
}(Ternary operators for keeping just a single return here would be bad for readability IMO, as some conditions aren't entirely trivial.) | |
| lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp | ||
|---|---|---|
| 803–804 | Strictly speaking, those should not use else either, but I don't think that's super important here, so you can leave it as they are now if you want. I'd be fine with a ternary operator too, as those conditions are not _that_ complicated... | |
Now that this is a return, you don't need the else as per http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return.