Page MenuHomePhabricator

Update __VERSION__ to remove the hardcoded 4.2.1 version
ClosedPublic

Authored by sylvestre.ledru on Jun 8 2019, 7:40 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sylvestre.ledru created this revision.Jun 8 2019, 7:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2019, 7:40 AM
xbolva00 added a subscriber: xbolva00.EditedJun 8 2019, 7:47 AM

Btw, what about this code

// Currently claim to be compatible with GCC 4.2.1-5621, but only if we're
// not compiling for MSVC compatibility
Builder.defineMacro("__GNUC_MINOR__", "2");
Builder.defineMacro("__GNUC_PATCHLEVEL__", "1");
Builder.defineMacro("__GNUC__", "4");

?

update of the release notes

Btw, what about this code

// Currently claim to be compatible with GCC 4.2.1-5621, but only if we're
// not compiling for MSVC compatibility
Builder.defineMacro("__GNUC_MINOR__", "2");
Builder.defineMacro("__GNUC_PATCHLEVEL__", "1");
Builder.defineMacro("__GNUC__", "4");

?

Dunno, what do you think @rnk ?

hubert.reinterpretcast added inline comments.
docs/ReleaseNotes.rst
79 ↗(On Diff #203693)

Use code font for __VERSION__.

lib/Frontend/InitPreprocessor.cpp
607 ↗(On Diff #203693)

s/as/for/; s/gcc/GCC/;
Add a period to the end of the sentence.

608 ↗(On Diff #203693)

s/returns/return/; s/clang/Clang/;

Fix the typo (thanks hubert)

sylvestre.ledru marked 2 inline comments as done.Jun 8 2019, 8:20 AM
docs/ReleaseNotes.rst
79 ↗(On Diff #203695)

I think this needs double backquotes on both sides.

fix rst syntax

sylvestre.ledru marked 2 inline comments as done.Jun 8 2019, 10:56 AM
lebedev.ri added inline comments.
lib/Frontend/InitPreprocessor.cpp
610 ↗(On Diff #203700)

So how does the entire __VERSION__ macro looks like now?

sylvestre.ledru marked an inline comment as done.Jun 8 2019, 11:47 AM
sylvestre.ledru added inline comments.
lib/Frontend/InitPreprocessor.cpp
610 ↗(On Diff #203700)
#define __VERSION__ "9.0.0 Compatible Clang 9.0.0 (trunk 362877)"

instead of

#define __VERSION__ "4.2.1 Compatible Clang 9.0.0 (trunk 362877)"
clang -dM -E -xc - < /dev/null

to see it

Personally, I think it's safe to do this. I believe this "4.2.1" compat thing was something Apple added as part of their switch from GCC, and then llvm-gcc, over to Clang, and I think most of the code they care about probably doesn't rely on __VERSION__ anymore. In any case, we should let @dexonsmith know.

As an alternative, maybe we should just stop defining the __VERSION__ macro. It's ambiguous. What is it the version of? The compiler, or the standard library? What gives the compiler the right to take some identifier, even in the implementer's namespace, and use it without putting it in their namespace by adding clang or GNUC to it? We already define __clang_version__, which has the same information.

lib/Frontend/InitPreprocessor.cpp
610 ↗(On Diff #203700)

If we're going to make this change, I don't think we need to say $VER Compatible Clang $VER, we can just say Clang $VER (trunk $REVISION), so this can be simplified to getClangFullCPPVersion(). For a vendor like Apple, it will become Apple Clang N.M.Q, which is probably fine. @dexonsmith

I will wait for @dexonsmith 's opinion

I think I'd be a little more comfortable dropping __VERSION__ entirely. There's already a way to access the clang version, and anyone with code that relies on "4.2.1" can just add a -D__VERSION__=... to their build rules to keep existing behaviour.

FWIW, I feel similarly about -dumpversion (better to remove it than change it).

FWIW, I feel similarly about -dumpversion (better to remove it than change it).

What does CMake use to detect compiler (well, clang) version?

FWIW, I feel similarly about -dumpversion (better to remove it than change it).

What does CMake use to detect compiler (well, clang) version?

Spoke too soon, clearly not that.

@rnk how do you feel about removing both? I can take care of that if you want

rnk added a comment.Jun 27 2019, 4:19 PM

I'd be in favor of removing them.

Actually, not sure it is a good idea to remove them: https://reviews.llvm.org/D64062#1566460

Implement the change (just update VERSION)

rnk accepted this revision.Jul 15 2019, 10:35 AM

lgtm

This revision is now accepted and ready to land.Jul 15 2019, 10:35 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 10:47 AM

Some ICC behaviour..

icc -dumpversion
19.0.3.199

printf("%d %d %s", GNUC, GNUC_MINOR, VERSION);
./a.out
7 4 Intel(R) C++ gcc 7.4 mode

Should we bump GNUC, GNUC_MINOR too?

rnk added a comment.Jul 29 2019, 1:52 PM

Should we bump GNUC, GNUC_MINOR too?

We discussed this two weeks ago:
http://lists.llvm.org/pipermail/cfe-dev/2019-July/062890.html
It's unlikely that we will make any policy change before 9.0.