Page MenuHomePhabricator

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

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



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
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

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).


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, 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.


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


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.