Page MenuHomePhabricator

[llvm-ar] Update error messages and tests as per latest preferred style
ClosedPublic

Authored by sameerarora101 on May 29 2020, 3:05 PM.

Details

Summary

It updates two error messages under performOperation
in the file llvm-ar.cpp. Furthermore, it also updates
tests that print out these error msgs:
llvm/test/Object/ar-create.test and
llvm/test/tools/llvm-ar/print.test.
Depends on D80838

Diff Detail

Event Timeline

sameerarora101 created this revision.May 29 2020, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2020, 3:05 PM
sameerarora101 edited the summary of this revision. (Show Details)May 29 2020, 3:17 PM
smeenai added inline comments.May 29 2020, 3:29 PM
llvm/tools/llvm-ar/llvm-ar.cpp
663

I'm guessing this was done by the linter as part of its formatting? It's a valid change, but it shouldn't be part of this diff.

Depending on how people who contribute to llvm-ar (e.g. @MaskRay) feel, we can either commit the formatting changes separately or just undo them (and ignore any lint warnings about them).

1031

Ditto here.

Add lower case i to the test message in print.test

Seems reasonable to me, but could you clarify why you are making the change? What is the latest "preferred style" you mean here?

@jhenderson There were two types of error messages being thrown before: error loading ... and unable to load ... So the idea is to make the error messages consistent (instead of having 2 different formats). Furthermore, since unable to load ... was updated more recently and is the preferred style going forward (as per @smeenai ), we should change the former to match the new message.

add colon to test titles.

As we discussed in https://bugs.llvm.org/show_bug.cgi?id=40244, some of the error messages here were still inconsistent, and this makes them consistent (with the style that was decided on in D67038).

jhenderson accepted this revision.Jun 2 2020, 1:29 AM

LGTM, with the formatting changes undone, and the test changes rebased once the new tests from D80838 are moved out.

llvm/tools/llvm-ar/llvm-ar.cpp
663

My personal take: if you are making changes in the immediate area (e.g. within a couple of lines), then it's okay to make the formatting change, either as part of, or in addition to the current patch, but that doesn't apply here, so I think this and the other formatting change below should be reverted.

This revision is now accepted and ready to land.Jun 2 2020, 1:29 AM

Rebasing on new D80838.

MaskRay accepted this revision.Jun 2 2020, 2:45 PM

Thanks!

jhenderson accepted this revision.Jun 3 2020, 12:15 AM

Looks good again, once the test is fixed in the parent revision.

Rebase again on D80838.

MaskRay accepted this revision.Jun 3 2020, 12:37 PM

Rebase again on D80838: splitting the directory and permission cases into separate files.

jhenderson accepted this revision.Jun 5 2020, 12:00 AM

Rebasing on D80838: updating comment "with chmod".

NFC: adding branch (git).

This revision was automatically updated to reflect the committed changes.