Details
Diff Detail
Event Timeline
| lib/DebugInfo/DWARFDebugInfoEntry.cpp | ||
|---|---|---|
| 90 | Roll the "Val" variable into the if condition to reduce its scope: if (Optional<uint64_t> Val = formValue.getAsUnsignedConstant()) Name = AttributeValueString(attr, Val.getValue()); (personally I prefer using the pointer-like interface of Optional (so I would write "*Val" rather than "Val.getValue()") but opinions differ and there's no solid convention in the codebase so far - so do whichever you prefer, I just mention it in case you aren't aware of it) | |
| 96 | Mistaken indentation here? (looks like a 4 space instead of a 2, but it might be the review software being weird) | |
| lib/Support/Dwarf.cpp | ||
| 799 | As a follow-up, if you like, all these functions should /probably/ be modified to return StringRef, that way we don't have to compute their length later on (or do null-test walks) in printing functions, etc. | |
| 799 | Should the type of Attr be something more type-safe? (the actual enum of DWARF attributes) | |
| 829 | This line is unreachable (so far as I can see?) and should be deleted. (alternatively you could leave this one and remove the default case from the switch) | |
| lib/DebugInfo/DWARFDebugInfoEntry.cpp | ||
|---|---|---|
| 90 | Will do. I wasn't aware of Optional::operator*. I'll use it. | |
| 96 | No the indentation is really off. | |
| lib/Support/Dwarf.cpp | ||
| 799 | I first wrote it like that, but I was finding the cast at the call site awkward. I can add it back, but we have no guarantee that the data we've read from the DWARF file will actually be a known Attribute, thus I find it more accurate to allow all values. | |
| 829 | I actually wondered while writing the code if all supported compilers could correctly do the control flow analysis that proves that the second return is unnecessary. I added the default case to prevent warnings about missed cases when I was using a typed enum Attr, but now the default case can just go away. | |
Looks good with your responses (& implied/pending updates) to the last round of comments, please commit once you've made those changes.
| lib/Support/Dwarf.cpp | ||
|---|---|---|
| 799 | Fair enough. | |
Roll the "Val" variable into the if condition to reduce its scope:
if (Optional<uint64_t> Val = formValue.getAsUnsignedConstant())
(personally I prefer using the pointer-like interface of Optional (so I would write "*Val" rather than "Val.getValue()") but opinions differ and there's no solid convention in the codebase so far - so do whichever you prefer, I just mention it in case you aren't aware of it)