This patch intends to be an NFC-patch. Test cases will be added in a follow-up patch.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
To silence all the clang-tidy warnings, I'd be happy if you want to do a bulk renaming of the functions (to emitDebug*) as a separate commit before this patch. Since you're going to modify all those lines in this patch anyway, it'll not make git blame any worse, which is usually the motivation behind not renaming things.
That being said, do you actually need to make all the functions return Error? Do you anticipate them all getting reporting some form of error? If you don't, making them return Error complicates the usage and adds some code paths (i.e. a function returning a failed Error) we can't test, because they are dead. Perhaps it would be better to only modify the signature of EmitDebugRanges, since that's where you need the error reported from.
llvm/lib/ObjectYAML/ELFEmitter.cpp | ||
---|---|---|
894–896 | Rather than just writing handleAllErrors here, I'd move this into a helper function alongside reportError, probably as a reportError overload that takes Error. That would be useful going forwards. |
Sure, I will do it later.
That being said, do you actually need to make all the functions return Error? Do you anticipate them all getting reporting some form of error? If you don't, making them return Error complicates the usage and adds some code paths (i.e. a function returning a failed Error) we can't test, because they are dead. Perhaps it would be better to only modify the signature of EmitDebugRanges, since that's where you need the error reported from.
Yes, I need to, but I'm not sure if I really need to. There's a helper function called EmitDebugSectionImpl that takes EmitFuncType as the parameter, where EmitFuncType can be EmitDebugStrings, EmitDebugARanges etc. The EmitDebugXXX functions have to have same return type (See my inline comment).
llvm/lib/ObjectYAML/DWARFEmitter.cpp | ||
---|---|---|
333 | The EmitDebugXXXX functions have to have same type. |
Okay, approach looks fine. Just my comments about function renaming and the error handler to address then.
llvm/lib/ObjectYAML/DWARFEmitter.cpp | ||
---|---|---|
333 | Right, I see, thanks! |
llvm/lib/ObjectYAML/ELFEmitter.cpp | ||
---|---|---|
863 | My concern this starts to look a bit bulky. Perhaps something like the following would be better? uint64_t BeginOffset = OS.tell(); Error Err = Error::success(); if (Name == ".debug_str") Err = DWARFYAML::EmitDebugStr(OS, DWARF); else if (Name == ".debug_aranges") Err = DWARFYAML::EmitDebugAranges(OS, DWARF); ... if (Err) return std::move(Err); |
llvm/lib/ObjectYAML/DWARFEmitter.cpp | ||
---|---|---|
411–431 | I think you could simplify this slightly: Error Err = emitDebugSectionImpl(DI, &DWARFYAML::emitDebugInfo, "debug_info", DebugSections)); | |
llvm/lib/ObjectYAML/ELFEmitter.cpp | ||
863 | Using joinErrors here and below doesn't make much sense, since there is only a single error to worry about. However, I believe you can't just assign to Err here either, as the original Err will be unchecked (even Error::success() needs to be checked before being overwritten/destroyed). I think the thing to do is close to what @grimar suggested, but with one additional line: Error Err = Error::success(); cantFail(Err); // Mark as checked before assigning. if (Name == ".debug_str") Err = DWARFYAML::EmitDebugStr(OS, DWARF); else if (Name == ".debug_aranges") Err = DWARFYAML::EmitDebugAranges(OS, DWARF); ... if (Err) return std::move(Err); | |
llvm/lib/ObjectYAML/MachOEmitter.cpp | ||
289–300 | Same comment as above - all the joinErrors calls don't need to happen, since there is only ever one Error to worry about. |
Address comments.
Thanks for reviewing!
llvm/lib/ObjectYAML/ELFEmitter.cpp | ||
---|---|---|
863 | Oh, thanks a lot. At first, I don't know how to make the Error::success() get checked. So I use joinErrors() everywhere. |
llvm/lib/ObjectYAML/ELFEmitter.cpp | ||
---|---|---|
863 |
I think the following code will invoke and trigger the original Err to be checked: /// Move-construct an error value. The newly constructed error is considered /// unchecked, even if the source error had been checked. The original error /// becomes a checked Success value, regardless of its original state. Error(Error &&Other) { setChecked(true); *this = std::move(Other); } |
llvm/lib/ObjectYAML/ELFEmitter.cpp | ||
---|---|---|
863 |
Am I wrong? |
llvm/lib/ObjectYAML/ELFEmitter.cpp | ||
---|---|---|
863 | Ah, I see I am wrong now. Nevermind. |
The EmitDebugXXXX functions have to have same type.