Page MenuHomePhabricator

remove redundant LLVM version from version string when setting CLANG_VENDOR
ClosedPublic

Authored by nickdesaulniers on Wed, Nov 6, 3:38 PM.

Details

Summary

When downstream LLVM distributions (like AOSP) set the CLANG_VENDOR
cmake variable, the version string printed by the clang driver looks
like:

$ clang --version
[CLANG_VENDOR] clang version X.X.X ([CLANG_REPOSITORY_STRING] sha) (based on LLVM X.X.X)

Rather than the more standard:
$ clang --version
clang version X.X.X ([CLANG_REPOSITORY_STRING] sha)

Based on feedback the the version string is a little long, the trailing
"(based on LLVM X.X.X)" is redundant and makes less sense after moving
LLVM to the monorepo. And it is only added should vendors set the cmake
variable CLANG_VENDOR. Let's remove it.

Diff Detail

Event Timeline

nickdesaulniers created this revision.Wed, Nov 6, 3:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 6, 3:38 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nickdesaulniers marked an inline comment as done.Wed, Nov 6, 3:40 PM
nickdesaulniers added inline comments.
clang/lib/Basic/Version.cpp
132

Looks like this is the sole use of BACKEND_PACKAGE_STRING, might as well remove that, too.

nickdesaulniers marked an inline comment as done.Wed, Nov 6, 3:41 PM
nickdesaulniers added inline comments.
clang/lib/Basic/Version.cpp
132

er, nvm, was running grep in my build dir.

efriedma added a subscriber: efriedma.

I think the reason it's written this way is that CLANG_VERSION_STRING might not correspond to an LLVM version? For example, Apple has its own versioning scheme.

I think the reason it's written this way is that CLANG_VERSION_STRING might not correspond to an LLVM version? For example, Apple has its own versioning scheme.

Line 128 is already printing that info though, isn't it?

+1, Apple's clang has an #ifdef 0 around this code.

clang/lib/Basic/Version.cpp
143

This use of CLANG_VENDOR should be removed as well presumably.

arphaman added inline comments.Thu, Nov 7, 10:39 AM
clang/lib/Basic/Version.cpp
143

Please ignore my comment. I missed the fact that this output is still needed to match the CLANG_VENDOR use above that's not removed.

nickdesaulniers marked 2 inline comments as done.Thu, Nov 7, 12:27 PM

+1, Apple's clang has an #ifdef 0 around this code.

Is that +1 in favor of the patch as is, or +1 to @eli.friedman 's comment?

Just grabbed a version string off a MBP:

$ clang --version
Apple LLVM version 10.0.1 (clang-1001.0.46.4)
Target: x86_64-apple-darwin18.7.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

So it looks like the Apple distribution has a quite different implementation of this method.

efriedma accepted this revision.Thu, Nov 7, 2:27 PM

If we expect that CLANG_VERSION_STRING and BACKEND_PACKAGE_STRING are going to be the same, then the extra note isn't really buying us anything. LGTM

This revision is now accepted and ready to land.Thu, Nov 7, 2:27 PM

Thanks for the review, leaving open for final comments, will merge tomorrow.

FWIW, I think we can now purge "svn" from BACKEND_PACKAGE_STRING and other places in the tree.

This revision was automatically updated to reflect the committed changes.