This is an archive of the discontinued LLVM Phabricator instance.

[llvm-rc] Support '--' for delimiting options from input paths
ClosedPublic

Authored by mstorsjo on Jan 15 2019, 1:46 PM.

Details

Summary

This allows avoiding conflicts between paths that begin with the same chars as some llvm-rc options (which can be used with either slashes or dashes).

It turned out that handling of -- for separating options from input paths isn't a built-in feature in the Options library, but within e.g. clang it's handled manually as part of a loop that iterates over input arguments.

Diff Detail

Event Timeline

mstorsjo created this revision.Jan 15 2019, 1:46 PM
zturner added inline comments.Jan 15 2019, 1:54 PM
tools/llvm-rc/llvm-rc.cpp
91

Should this be " -- "? I don't think llvm-rc.exe foo--bar should trigger this, that's one argument that happens to have 2 dashes.

mstorsjo marked an inline comment as done.Jan 15 2019, 1:56 PM
mstorsjo added inline comments.
tools/llvm-rc/llvm-rc.cpp
91

Yes, but this is strcmp on a per-argument basis, not strstr on the concatenated command line. strcmp("foo--bar", "--") doesn't return 0.

zturner accepted this revision.Jan 15 2019, 2:08 PM
zturner added inline comments.
tools/llvm-rc/llvm-rc.cpp
91

Ahh, right. Not sure how I missed that. I think it would still compile if you made the lambda function take a StringRef and then just wrote return str == "--";. If that works, then I'd request that change. If it doesn't though, LGTM as-is.

This revision is now accepted and ready to land.Jan 15 2019, 2:08 PM
mstorsjo marked an inline comment as done.Jan 15 2019, 2:12 PM
mstorsjo added inline comments.
tools/llvm-rc/llvm-rc.cpp
91

Right - yes, that works. Will change it that way (and rename str to Str) before committing.

mstorsjo updated this revision to Diff 181885.Jan 15 2019, 2:13 PM

Using a lambda that takes StringRef, renamed the lambda parameter to match coding conventions. Will commit tomorrow.

This revision was automatically updated to reflect the committed changes.
eugenis added inline comments.
llvm/trunk/test/tools/llvm-rc/tag-accelerators.test
82 ↗(On Diff #181983)

Hi! I think you missed a spot here (and in a hundred other places).
I'll upload a patch.

Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 6:00 PM