Page MenuHomePhabricator

Add support to print version.
ClosedPublic

Authored by shankarke on Oct 6 2014, 9:57 PM.

Diff Detail

Event Timeline

shankarke updated this revision to Diff 14489.Oct 6 2014, 9:57 PM
shankarke retitled this revision from to Add support to print version..
shankarke updated this object.
shankarke edited the test plan for this revision. (Show Details)
shankarke added a project: lld.
shankarke added a subscriber: Unknown Object (MLST).
shankarke updated this revision to Diff 14490.Oct 6 2014, 10:14 PM

Update change to fix few minor nits.

ruiu edited edge metadata.Oct 7 2014, 10:51 AM

Please write more descriptive commit message for this patch.
Something like "Add -version option to the universal driver to
print out LLD version and SVN revision number.".

CMakeLists.txt
99

I don't think we need this. It's always https://llvm.org/svn/llvm-project/lld/trunk.

include/lld/Config/Version.h
1

lib/Config/Version.h

45

We could print out revision number along with the version number like "version 3.5 (r205863)". I don't see the reason to print out repository name. Remove it?

lib/Config/Version.cpp
1

lib/Config/Version.cpp.

24

It's always defined, no?

32

Ditto

39

Remove?

59

Ditto

lib/Driver/UniversalDriver.cpp
194

Remove space before :.

shankarke added inline comments.Oct 7 2014, 10:57 AM
CMakeLists.txt
99

Its a standard thing followed by other projects like lldb/clang.

include/lld/Config/Version.h
1

Sure.

45

Same thing the style is followed by clang/lldb as well. so kept it consistent.

lldb --version 2>/dev/null
lldb version 3.6.0 ( https://llvm.org/svn/llvm-project/lldb/trunk revision 218537)

lib/Config/Version.cpp
24

The repository path and the version is empty on windows. Its a TODO.

lib/Driver/UniversalDriver.cpp
194

ok.

shankarke added inline comments.Oct 7 2014, 11:16 AM
include/lld/Config/Version.h
1

The file is an include file and is included by UniversalDriver, I dont think we should keep it in lib.

shankarke updated this revision to Diff 14521.Oct 7 2014, 11:30 AM
shankarke edited edge metadata.

address review comments.

ruiu added a comment.Oct 7 2014, 11:36 AM

Please update the commit message to be more descriptive.

CMakeLists.txt
99

But clang doesn't have -repository-version option, no?

include/lld/Config/Version.h
1

What I mean is to update this line to match the path. I should've write "include/lld/Config/Version.h". Sorry.

45

OK, then let's use the same format as LLDB, instead of adding two options, -version and -repository-version, and remove remove -repository-version.

shankarke added inline comments.Oct 7 2014, 11:51 AM
CMakeLists.txt
99

It looks that option has disappeared from clang, I had seen that a long long time back.

include/lld/Config/Version.h
1

Updated to mention this properly in the new patch.

45

Ok.

lib/Config/Version.cpp
1

ok.

I have changed the commit message but for some reason, its not appearing as part of the new diff. You will see it when I commit.

This is how it looks just FYI.

Add support to print version.

Summary: This adds support in the UniversalDriver to print the linker version and the
repository version.

Test Plan: A driver test is added

Differential Revision: http://reviews.llvm.org/D5641
shankarke updated this revision to Diff 14524.Oct 7 2014, 12:03 PM

address review comments.

ruiu accepted this revision.Oct 7 2014, 12:07 PM
ruiu edited edge metadata.

LGTM with this nit.

lib/Config/Makefile
1

Remove "lld/" from this line.

This revision is now accepted and ready to land.Oct 7 2014, 12:07 PM
shankarke updated this revision to Diff 14550.Oct 7 2014, 8:45 PM
shankarke edited edge metadata.

Address review comments and commit.

shankarke closed this revision.Oct 7 2014, 8:48 PM

For some reason, arc committed the patch with my old description. I don't have any clue why it did that and didn't use my latest change in the commit description.