Page MenuHomePhabricator

[llvm-readobj] - Remove 'error(Error EC)' helper.
ClosedPublic

Authored by grimar on Aug 9 2019, 5:39 AM.

Details

Summary

We do not need it. I replaced it with
reportError(StringRef Input, Error Err)
almost without problems.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Aug 9 2019, 5:39 AM
MaskRay added inline comments.Aug 9 2019, 6:01 AM
tools/llvm-readobj/COFFDumper.cpp
1922 ↗(On Diff #214365)

reportError(..., codeview::visitTypeStream(..))

tools/llvm-readobj/ELFDumper.cpp
4417 ↗(On Diff #214365)

if (Err) can be deleted

4429 ↗(On Diff #214365)

if (Err) can be deleted

tools/llvm-readobj/llvm-readobj.cpp
385 ↗(On Diff #214365)

reportError now checks if (!Err). Is it still appropriate to call it reportError? I want to hear other opinions.

grimar marked an inline comment as done.Aug 9 2019, 6:06 AM
grimar added inline comments.
tools/llvm-readobj/llvm-readobj.cpp
385 ↗(On Diff #214365)

Previously error(Error EC) did that. I can probably remove if (!Err) check to avoid the confusion.

jhenderson added inline comments.Aug 9 2019, 6:18 AM
tools/llvm-readobj/llvm-readobj.cpp
385 ↗(On Diff #214365)

I'd probably replace the if (!Err) check with an equivalent assertion. I think it's natural to write code like:

if (Error E = doSomething())
  reportError(File, std::move(E));

whereas the following looks weird to me:

reportError(doSomethingWhichWillSucceed());

An alternative name might be "checkError" if you didn't do that.

MaskRay added inline comments.Aug 9 2019, 6:22 AM
tools/llvm-readobj/llvm-readobj.cpp
385 ↗(On Diff #214365)

checkError looks good to me.

grimar marked an inline comment as done.Aug 9 2019, 6:27 AM
grimar added inline comments.
tools/llvm-readobj/llvm-readobj.cpp
385 ↗(On Diff #214365)

I'd probably stick to reportError version. It looks simpler and more natural to use for me.

MaskRay added inline comments.Aug 9 2019, 6:31 AM
tools/llvm-readobj/llvm-readobj.cpp
385 ↗(On Diff #214365)

I think I slightly prefer Error being the first argument. This allows to use default arguments for other fields if we supply more arguments, e.g.:

checkError(Error E, StringRef FileName, StringRef ArchiveName,
             StringRef ArchitectureName = StringRef());
grimar updated this revision to Diff 214796.Aug 13 2019, 3:41 AM
grimar marked an inline comment as done.
  • Updated in according to the discussion.
tools/llvm-readobj/llvm-readobj.cpp
385 ↗(On Diff #214365)

Ok, I made Error to be the first argument. I keeped reportError name and added an assert
to make sure it is used with if:

if (Err)
  reportError(Err, ...);
MaskRay accepted this revision.Aug 13 2019, 3:51 AM
This revision is now accepted and ready to land.Aug 13 2019, 3:51 AM
jhenderson added inline comments.Aug 13 2019, 3:59 AM
tools/llvm-readobj/COFFDumper.cpp
1170 ↗(On Diff #214796)

Is EC an Error or std::error_code? If the latter, the move below is unnecessary.

1271 ↗(On Diff #214796)

Same comment here and immediately below as above.

grimar marked 3 inline comments as done.Aug 13 2019, 4:05 AM
grimar added inline comments.
tools/llvm-readobj/COFFDumper.cpp
1170 ↗(On Diff #214796)

It's an Error. EC is a bad original name, I can change it to E before the commit.

1271 ↗(On Diff #214796)

It is also an Error returned here.

grimar marked 2 inline comments as done.Aug 13 2019, 4:06 AM
grimar added inline comments.
tools/llvm-readobj/COFFDumper.cpp
1170 ↗(On Diff #214796)

Will also replace auto with an explicit type.

jhenderson accepted this revision.Aug 13 2019, 4:35 AM

LGTM, with the suggestions.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 5:08 AM