Bug 84974 - WARN_UNUSED_RETURN should be defined for Clang
Summary: WARN_UNUSED_RETURN should be defined for Clang
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tom Zakrajsek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-26 10:19 PDT by Tom Zakrajsek
Modified: 2012-04-30 11:58 PDT (History)
3 users (show)

See Also:


Attachments
test case snippet (383 bytes, application/octet-stream)
2012-04-26 10:24 PDT, Tom Zakrajsek
no flags Details
Patch (1.01 KB, patch)
2012-04-26 10:30 PDT, Tom Zakrajsek
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Zakrajsek 2012-04-26 10:19:13 PDT
Since Clang supports warn_unused_result attribute, the WARN_UNUSED_RETURN macro in Compiler.h (currently in Source/WTF/wtf) should reflect that.
Comment 1 Tom Zakrajsek 2012-04-26 10:24:18 PDT
Created attachment 139021 [details]
test case snippet
Comment 2 Tom Zakrajsek 2012-04-26 10:30:16 PDT
Created attachment 139024 [details]
Patch
Comment 3 Tom Zakrajsek 2012-04-27 07:45:13 PDT
Anders, would you mind reviewing this at your convenience.  Maciej sold you out as the clang expert. :)
Comment 4 Alexey Proskuryakov 2012-04-27 09:02:37 PDT
Comment on attachment 139024 [details]
Patch

What's our general approach to these macros - do we want COMPILER(GCC) to be true in clang (which reportedly defines __GCC__, at least on some systems)? This is related to bug 85018.
Comment 5 Tom Zakrajsek 2012-04-30 08:19:46 PDT
Anders told me in IRC that he does not think clang imply COMPILER(GCC)==true.
Comment 6 Alexey Proskuryakov 2012-04-30 09:48:22 PDT
I think that it does imply that, and we even have code respecting this in Compiler.h:

$ clang -dM -E - < /dev/null
...
#define __GNUC__ 4


#if defined(__GNUC__) && !COMPILER(RVCT)
#define WTF_COMPILER_GCC 1
#define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)
#define GCC_VERSION_AT_LEAST(major, minor, patch) (GCC_VERSION >= (major * 10000 + minor * 100 + patch))

/* Specific compiler features */
#if !COMPILER(CLANG) && GCC_VERSION_AT_LEAST(4, 6, 0) && defined(__GXX_EXPERIMENTAL_CXX0X__)
#define WTF_COMPILER_SUPPORTS_CXX_NULLPTR 1 
#endif

Note the !COMPILER(CLANG) clause inside defined(__GNUC__)
Comment 7 Tom Zakrajsek 2012-04-30 11:39:33 PDT
I'm holding off on landing this for now.  For reasons that i can't explain, my test case now behaves as if COMPILER(GCC) is already true for clang, and in mapping it out, that's how the code reads too.  This was not the behavior i saw last week, but I see no code change in the logs that would effect this.  I want to understand this file better before making any change.

The definitions are inconsistent either way.  For example, RVCT defines __GNUC__ but we specifically avoid defining COMPILER(GCC) for that case, whereas we appear to define it for other GCC compatible compilers (clang and mingw at least).

Personally, I think the best design would be to have COMPILER(x) values not overlap, but let COMPILER_SUPPORTS(x) handle overlaps where needed.  For now, the behavior I was seeing that prompted the change is not reproducible, so I will spend some more time getting familiar with this code.
Comment 8 Alexey Proskuryakov 2012-04-30 11:58:43 PDT
> Personally, I think the best design would be to have COMPILER(x) values not overlap

That's what I think too, but I've never been involved in designing these macros, and may well overlook something important.