This is an archive of the discontinued LLVM Phabricator instance.

[llvm-ar] Support * as comment char in MRI scripts
ClosedPublic

Authored by mstorsjo on Aug 28 2018, 12:40 AM.

Details

Summary

MRI scripts have two comment chars, * and ;, but only the latter was supported before.

Also allow leading spaces before comment chars (and before any command string), and allow comments after a command.

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 28 2018, 12:40 AM
pcc added inline comments.Aug 29 2018, 12:40 PM
tools/llvm-ar/llvm-ar.cpp
819

What should happen if a line contains both a command and a comment?

mstorsjo added inline comments.Aug 29 2018, 12:44 PM
tools/llvm-ar/llvm-ar.cpp
819

I'm not sure I'm following, can you elaborate?

If the first word of the line starts with either ; or *, the whole line is a comment.

pcc added inline comments.Aug 29 2018, 12:53 PM
tools/llvm-ar/llvm-ar.cpp
819

I mean what should be the interpretation of the line addlib foo.a ; comment? Does it add a library named foo.a or foo.a ; comment or is it an error?

mstorsjo added inline comments.Aug 29 2018, 1:11 PM
tools/llvm-ar/llvm-ar.cpp
819

Oh, good point. When checking with GNU 'ar', it creates a library named foo.a only.

I'll see if I can amend the code to handle this as well.

mstorsjo updated this revision to Diff 163188.Aug 29 2018, 2:00 PM
mstorsjo edited the summary of this revision. (Show Details)

Handle the comment char appearing anywhere in a command line - by discarding anything after the comment char.

mstorsjo retitled this revision from [llvm-ar] Support * as comment char in MIR scripts to [llvm-ar] Support * as comment char in MRI scripts.Aug 29 2018, 2:03 PM
pcc added inline comments.Sep 4 2018, 6:24 PM
tools/llvm-ar/llvm-ar.cpp
807

Don't pass /*SkipBlanks*/ true, ';' here because you're already handling them below.

810

Perhaps more succinctly Line = Line.split(';').first;?

mstorsjo added inline comments.Sep 4 2018, 10:15 PM
tools/llvm-ar/llvm-ar.cpp
807

Ok, will change.

810

Ah, of course - yes.

mstorsjo updated this revision to Diff 163973.Sep 4 2018, 10:16 PM

Avoided needless use of std::tie, passing SkipBlanks = false as we handle that skipping locally anyway now.

pcc added a comment.Sep 6 2018, 10:55 AM

LGTM

tools/llvm-ar/llvm-ar.cpp
809

Move this decl next to line 815.

pcc accepted this revision.Sep 6 2018, 10:56 AM
This revision is now accepted and ready to land.Sep 6 2018, 10:56 AM
This revision was automatically updated to reflect the committed changes.
mstorsjo added inline comments.Sep 6 2018, 12:02 PM
tools/llvm-ar/llvm-ar.cpp
809

Sorry, I missed this comment before - will fix this as well.