This is an archive of the discontinued LLVM Phabricator instance.

Enable constexpr on Visual Studio 2015, add support for two equivalent attributes
ClosedPublic

Authored by ariccio on Jan 30 2016, 4:59 PM.

Details

Summary

I'm not sure who owns Compiler.h, so I'll add people from the revision log. If it's not correct, please add the correct subscriber?

This patch enables constexpr on Visual Studio 2015 by adding || LLVM_MSC_PREREQ(1900) to the preprocessor #if statement. Since VS2013 doesn't support constexpr, that's purposely excluded. The LLVM_CONSTEXPR macro is used in ~25 places.

I also added the MSVC/SAL equivalent of:

  • __attribute__((__warn_unused_result__)) as an LLVM_ATTRIBUTE_UNUSED_RESULT definition
  • __attribute__((returns_nonnull)) as an LLVM_ATTRIBUTE_RETURNS_NONNULL definition

...in case anybody ever decides to run /analyze on LLVM (probably myself, if anybody)

Diff Detail

Event Timeline

ariccio updated this revision to Diff 46478.Jan 30 2016, 4:59 PM
ariccio retitled this revision from to Enable constexpr on Visual Studio 2015, add support for two equivalent attributes.
ariccio updated this object.
ariccio added a comment.EditedJan 30 2016, 9:56 PM

Everything SEEMS good, although I need to investigate two new failing tests?

(some time later)

I filed a bug. https://llvm.org/bugs/show_bug.cgi?id=26399 Everything should be good.

ariccio updated this object.Jan 30 2016, 10:18 PM
ariccio added reviewers: bogner, davidxl, aaron.ballman.
ariccio added a subscriber: llvm-commits.
aaron.ballman added subscribers: majnemer, rnk.

Generally LGTM, but adding @rnk and @majnemer for an extra opinion on MSVC's constexpr-readiness.

C:/LLVM/llvm/include/llvm/Support/Compiler.h
99

I think this is reasonable, but we may still get some failures depending on usage. MSVC's constexpr support is drastically improved in MSVC 2015, but is not C++14 constexpr, it's C++11 constexpr.

rnk added inline comments.Feb 1 2016, 8:50 AM
C:/LLVM/llvm/include/llvm/Support/Compiler.h
132

This is provided by sal.h, right? You should probably add an include like this at the top:

#ifdef _MSC_VER
#include <sal.h>
#endif

Grr. I need to "submit" comments.

C:/LLVM/llvm/include/llvm/Support/Compiler.h
132

It is provided by sal.h, but it's like size_t: you generally don't need to #include <stddef.h> to use size_t, or #include <cstddef>, for std::size_t - they're usually included by default or built in.

If you want to be super strict about standards conformance - which is a totally noble goal - then lemme know and I will go ahead and actually #include <sal.h>.

Responded to single comment.

C:/LLVM/llvm/include/llvm/Support/Compiler.h
99

That's a valid concern, but everything built fine.

...so I guess LLVM doesn't yet use C++14 constexpr?

aaron.ballman added inline comments.Feb 3 2016, 7:58 AM
C:/LLVM/llvm/include/llvm/Support/Compiler.h
99

Probably not. I don't think this concern is a reason to *not* enable this functionality if our usages currently work as expected.

132

Since Compiler.h is a stand-alone header file, I think it should include sal.h. Good catch, @rnk!

ariccio updated this revision to Diff 46802.Feb 3 2016, 10:03 AM
ariccio edited edge metadata.
ariccio marked an inline comment as done.

Added #include <sal.h>.

ariccio marked 5 inline comments as done.Feb 3 2016, 10:04 AM
ariccio added inline comments.
C:/LLVM/llvm/include/llvm/Support/Compiler.h
132

Ok, will do.

132

Will upload patch as:

#if defined(_MSC_VER)
#include <sal.h>
#endif
ariccio marked 3 inline comments as done.EditedFeb 3 2016, 10:04 AM

Forgot to mark three things as done.

aaron.ballman accepted this revision.Feb 8 2016, 1:31 PM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Feb 8 2016, 1:31 PM

Pardon my ignorance, but what exactly does This revision is now accepted and ready to land. mean? I assume it's waiting on something?

No, I don't mean literally.

Pardon my ignorance, but what exactly does This revision is now accepted and ready to land. mean? I assume it's waiting on something?

No, I don't mean literally.

It means that the revision is no longer in review status and you are welcome to commit the patch to trunk. The next step is for you (or someone else) to commit these changes, and then you should watch for build bot failures (in IRC is preferred, though you will get an email from most bots if your patch is in a batch of commits that causes a failure), and watch for any post-commit review feedback (which generally comes in on either this thread, or the thread started by the automated commit email).

Pardon my ignorance, but what exactly does This revision is now accepted and ready to land. mean? I assume it's waiting on something?

No, I don't mean literally.

It means that the revision is no longer in review status and you are welcome to commit the patch to trunk. The next step is for you (or someone else) to commit these changes, and then you should watch for build bot failures (in IRC is preferred, though you will get an email from most bots if your patch is in a batch of commits that causes a failure), and watch for any post-commit review feedback (which generally comes in on either this thread, or the thread started by the automated commit email).

To be more explicit: if you don't have commit access, usually you need to ask here for someone to commit it for you.
To get commit access, see: http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

This revision was automatically updated to reflect the committed changes.

Nit, it says:

(authored by rnk)

...eh? rnk committed it, not authored it?

rnk edited edge metadata.Feb 10 2016, 4:24 PM

Yeah, unfortunately SVN just has committer information, not actual info about who wrote the patch, unlike git. Our policy has been to add a line like "Patch by Hacker Jenny" when committing on behalf of others, so people looking for authorship should know to look for that. If you click through to rL260410 you can see it says "Patch by Alexander Riccio".