Bug 126324 – configure check for clang version is incorrect

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jeremy Huddleston Sequoia]

Created attachment 220144 [details]
trivial patch

The logic to verify >= clang-3.2 is incorrect:
 1) It does not correctly check Apple clang versions
 2) It forces minor version >= 2 even when major version > 3 (eg 4.0 will fail)

Trivial patch attached

[Csaba Osztrogonác]

Please add changelog and set r? if you would like to ask for review.

[Jeremy Huddleston Sequoia]

Created attachment 223284 [details]
clang-check.patch

[WebKit Commit Bot]

Attachment 223284 [details] did not pass style-queue:


Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philip Chimento]

Created attachment 239190 [details]
Updated patch

The same logic error was made with GCC versions too. Here's an updated patch.

[Michael Catanzaro]

Note that the autotools build was removed for 2.6.

The version check is indeed incorrect and will break when GCC 5 is released next spring, but the proposed fix is wrong too. The right hand side of the || will always be short-circuited:

(__GNUC__ >= 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 7)

You want:

(__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 7)

The altered Clang version check is correct.

[Philip Chimento]

Created attachment 239312 [details]
Patch

[Philip Chimento]

Thanks for catching that. Here's another one. I know the autotools build is obsolete but it would be nice if this could be committed to the 2.4 branch.

[Philip Chimento]

Would it be possible to get this reviewed for 2.4.8?

[Philip Chimento]

Would it be possible to review this for 2.4.9?

[Michael Catanzaro]

Carlos, if you're planning to do another 2.4 release, can you take this patch please? We're going to use this patch in Fedora now that we've upgraded to the GCC 5.0 prerelease.

[Carlos Garcia Campos]

Yes, I will. I already had this in my todo list for the next 2.4, but please, remember that the best way to make sure I don't forget a bug/patch is adding it to the list of proposed merges in http://trac.webkit.org/wiki/WebKitGTK/2.4.x

[Philip Chimento]

Sure thing. I somehow thought that that page was only for reviewed patches, but I'll add them there in the future.

[Carlos Garcia Campos]

(In reply to comment #12)
> Sure thing. I somehow thought that that page was only for reviewed patches,
> but I'll add them there in the future.

Not necessarily, the rule might be something like this:

  - If it has landed, add it as proposed merge for next release
  - If it's important bug, add it as proposed merge for next release
  - Otherwise, or in doubt, add it as changes on track
  - When adding a landed patch, please use the trac changeset instead of the bug url

Note that I'll check every change before merging, so it's better to add something there even if you are not sure, that not adding it. But, please, make sure the change hasn't been merged yet (not all changes merged are in the wiki)

[Alberto Garcia]

I confirm that 2.4.8 builds fine with gcc 5 and these changes.

[Carlos Garcia Campos]

Committed <http://trac.webkit.org/changeset/182457>