This is an archive of the discontinued LLVM Phabricator instance.

Add cmd to compilation database file format
ClosedPublic

Authored by diltsman on Jun 10 2015, 11:11 AM.

Details

Reviewers
klimek
Summary

In the compilation database command must be escaped. This can cause problems on some systems (Windows). cmd is added as a list of command line arguments that will not be escaped.

Diff Detail

Event Timeline

diltsman updated this revision to Diff 27458.Jun 10 2015, 11:11 AM
diltsman retitled this revision from to Add cmd to compilation database file format.
diltsman updated this object.
diltsman edited the test plan for this revision. (Show Details)
diltsman added a subscriber: Unknown Object (MLST).

Small aside: phab helps mainly when context is added to the diff (see http://llvm.org/docs/Phabricator.html)
git: git diff -U999999 other-branch
svn: svn diff --diff-cmd=diff -x -U999999
(or you can use the arc diff command line tool)

Apart from that, my comment on the initial thread still stands:
I think we should name the list one "arguments" or (if you prefer) "args"; I think that makes a lot more sense: command: the shell command that is run, arguments: the command line arguments.

I did the rename to args, but apparently I uploaded the old patch file. New patch file coming shortly.

diltsman updated this revision to Diff 27539.Jun 11 2015, 1:09 PM

Sorry I missed the update - I still see "cmd" in the latest patch (note
that phab is currently down due to somebody trying to DDOS or hack it).

I'm trying to upload a new patch and I get an error from Phabricator:
'Unhandled Exception ("Exception")'.

Any ideas how to get this to work? I have attached the diff file.

diltsman updated this revision to Diff 31824.Aug 11 2015, 9:47 AM

Ok, we're very close now :) Thanks for taking the time to work through this!

../llvm/tools/clang/lib/Tooling/JSONCompilationDatabase.cpp
299 ↗(On Diff #31824)

Any reason we don't want to allow both, but prefer the arguments?

302–304 ↗(On Diff #31824)

Can we use for-range loops with auto?

diltsman updated this revision to Diff 31952.Aug 12 2015, 9:51 AM
diltsman marked an inline comment as done.
diltsman added inline comments.
../llvm/tools/clang/lib/Tooling/JSONCompilationDatabase.cpp
299 ↗(On Diff #31824)

I don't see how the arguments are preferred.

I was thinking that the user would either want to use the command line or list arguments. Is there a case where someone would want to do both?

Also, if we permit both, do we need to do any kind of uniquing?

302–304 ↗(On Diff #31824)

I changed two other locations to range-based for loops, too.

klimek added inline comments.Aug 13 2015, 1:07 AM
../llvm/tools/clang/lib/Tooling/JSONCompilationDatabase.cpp
295 ↗(On Diff #31952)

You're probably right. I guess people are not distributing clang tools (yet!), so we probably don't need tools like cmake to output both at the same time.

What I meant with "preferred" is that we could allow specifying both, and then we would use 'arguments', if specified, and otherwise fall back to 'command'. Does that make sense?

298–300 ↗(On Diff #31952)

Thanks!

diltsman updated this revision to Diff 32082.Aug 13 2015, 10:57 AM
diltsman marked an inline comment as done.

Arguments and Command can now be in the same compilation database for the same file. Arguments are preferred when both are present.

klimek accepted this revision.Aug 13 2015, 11:33 AM
klimek added a reviewer: klimek.

LG with a happy path test.

../llvm/tools/clang/unittests/Tooling/CompilationDatabaseTest.cpp
45 ↗(On Diff #32082)

Please add a happy path test, too.

This revision is now accepted and ready to land.Aug 13 2015, 11:33 AM
diltsman updated this revision to Diff 32099.Aug 13 2015, 3:27 PM
diltsman edited edge metadata.
diltsman marked an inline comment as done.

Added test for command and arguments in same object.
Fixed bug where parse failed if command and argument resolved to empty lists.

Cool, looks good! If you need me to land it let me know.

Yes, please land it.

Unfortunately I can't get the patch to apply cleanly due to different line endings.

Solved the line ending problem.

klimek closed this revision.Aug 14 2015, 2:58 AM

Submitted as r245036. Thanks!