Page MenuHomePhabricator

Introduce llvm/ADT/OptionSet.h utility class
Needs RevisionPublic

Authored by akyrtzi on Feb 16 2016, 8:22 PM.

Details

Summary

Introduces ‘OptionSet’ in llvm/ADT headers, which is a utility class that makes it convenient to work with enumerators representing bit options.

We have found it useful in the swift repo where it originates. Apple owns the copyright on the entire file as posted for review and can thus relicense under the LLVM license.

For example of its usefulness, if accepted I intend to use OptionSet in clang code, for 'SymbolRoleSet' here: http://llvm.org/svn/llvm-project/cfe/trunk/include/clang/Index/IndexSymbol.h.
This will improve type safety and make the code more readable, avoiding all the 'unsigned' casts such as this:

Roles |= (unsigned)SymbolRole::Implicit;

MSVC 2013 had some trouble with the code and there are parts ifdef’ed out for MSVC 2013.
Aaron was kind enough to offer feedback on the MSVC issues.
Per Jordan, the static assert that gives trouble for MSVC is just an enforced style check.

The specific MSVC issues were:

Compiler crash:
http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/9718
Compiler error in unit test:
http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/9720
Unit test failure:
http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/9721

Diff Detail

Repository
rL LLVM

Event Timeline

akyrtzi updated this revision to Diff 48146.Feb 16 2016, 8:22 PM
akyrtzi retitled this revision from to Introduce llvm/ADT/OptionSet.h utility class.
akyrtzi updated this object.
akyrtzi added reviewers: reames, chandlerc.
akyrtzi set the repository for this revision to rL LLVM.
akyrtzi removed rL LLVM as the repository for this revision.Feb 16 2016, 8:29 PM
akyrtzi added a subscriber: llvm-commits.
reames accepted this revision.Feb 22 2016, 2:06 PM
reames edited edge metadata.

LGTM w/minor comments.

include/llvm/ADT/OptionSet.h
51

How does this differ from the one above it?

55

Please add an assertion that Storage is a power of two inside this constructor. This is the only place we get to validate that Flag is actually a disjoint bitset.

74

Should this be a static_cast<T>? Otherwise, why have the type param?

99

minor: expression & in terms of &= or vice versa would make it more clear that both are correct (or equally wrong), but increasing test coverage. Its very, very likely that any decent compiler will elide the extra copies.

unittests/ADT/OptionSetTest.cpp
89

Seems like this could be handled just like the early constructable return.

This revision is now accepted and ready to land.Feb 22 2016, 2:06 PM
chandlerc added inline comments.Feb 22 2016, 2:16 PM
include/llvm/ADT/OptionSet.h
74

It causes 'sizeof(T)' to be dependent for the purpose of SFINAE.

At the point you've done this though, you might as well also tuck the enable_if into a template argument. You're already using the feature with an optional template argument here...

ham99922 requested changes to this revision.Jan 12 2019, 10:09 AM
ham99922 added a subscriber: ham99922.
This comment was removed by asl.
This revision now requires changes to proceed.Jan 12 2019, 10:09 AM
This comment was removed by asl.
asl added a subscriber: asl.Jan 14 2019, 5:29 AM

ConversionFunction