[llvm-dev] CommandLine -- ResetAllOptionOccurrences with cl::bits -- Bug or by design?

[Riyaz Puthiyapurayil via llvm-dev]

I didn't see a response. But I fixed this in our downstream code and it is working now as expected. It fixes two issues (1) clearing cl::bits and (2) allowing an optional storage location for cl::bits (the reinterpret_cast is wrong for an enum to unsigned cast; it should be a static_cast). If there is interest in fixing this upstream, I can send the following patch for review (note, the following is for our downstream branch but I can put together a patch for master along with some unit tests that expose the issue).

diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h
index 05374e3..712d6d3 100644
--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -1720,7 +1720,7 @@ template <class DataType, class StorageClass> class bits_storage {
   unsigned *Location = nullptr; // Where to store the bits...

   template <class T> static unsigned Bit(const T &V) {
-    unsigned BitPos = reinterpret_cast<unsigned>(V);
+    unsigned BitPos = static_cast<unsigned>(V);
     assert(BitPos < sizeof(unsigned) * CHAR_BIT &&
            "enum exceeds width of bit vector!");
     return 1 << BitPos;
@@ -1744,6 +1744,11 @@ public:

   unsigned getBits() { return *Location; }

+  void clear() {
+    if (Location)
+      *Location = 0;
+  }
+
   template <class T> bool isSet(const T &V) {
     return (*Location & Bit(V)) != 0;
   }
@@ -1767,6 +1772,8 @@ public:

   unsigned getBits() { return Bits; }

+  void clear() { Bits = 0; }
+
   template <class T> bool isSet(const T &V) { return (Bits & Bit(V)) != 0; }
};

@@ -1813,7 +1820,7 @@ class bits : public Option, public bits_storage<DataType, Storage> {
   void printOptionValue(size_t /*GlobalWidth*/, bool /*Force*/) const override {
   }

-  void setDefault() override {}
+  void setDefault() override { bits_storage<DataType, Storage>::clear(); }

   void done() {
     addArgument();

From: Riyaz Puthiyapurayil
Sent: Sunday, March 21, 2021 4:30 PM
To: llvm-dev at lists.llvm.org
Subject: CommandLine -- ResetAllOptionOccurrences with cl::bits -- Bug or by design?

I have some unit tests where I want to parse different command lines where some options use cl::bits with an enum type. I want to reset the option occurrences in each unit test and reinvoke the command line parser with a new command line. While ResetAllOptionOccurrences resets every other kind of option, it doesn't do so for cl::bits. Is this by design?

I see setDefaultValue of cl::bits is an empty function. Why doesn't it set the internal bit storage to 0 when ResetAllOptionOccurrences is called? I thought I could instead use an external storage and set that to 0. But that doesn't work either. It results in an error:

unsigned MyOptStorage;
static llvm::cl::bits<MyOpt, unsigned> MyOpts(
....
      llvm::cl::location(MyOptStorage),
...);

include/llvm/Support/CommandLine.h:1723:23: error: reinterpret_cast from 'MyOpt' to 'unsigned int' is not allowed
    unsigned BitPos = reinterpret_cast<unsigned>(V);

where 'MyOpt' is an enum type.

So how does one specify cl::bits with an external storage location if DataType is an enum? There is not much information in Command Line Reference Manual. Note that I used unsigned above just like the excerpt from the manual says:

The cl::bits class is the class used to represent a list of command line options in the form of a bit vector. It is also a templated class which can take up to three arguments:
namespace cl {
  template <class DataType, class Storage = bool,
            class ParserClass = parser<DataType> >
  class bits;
}
This class works the exact same as the cl::list class, except that the second argument must be of type unsigned if external storage is used.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210324/5eaf22c1/attachment.html>