[CommandLine] Set the 'ValueStr' with the selected argument

[bwendling]

Setting ValueStr allows us to query which option was selected.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Bill Wendling (bwendling)

Changes

Setting ValueStr allows us to query which option was selected.

---

Full diff: https://github.com/llvm/llvm-project/pull/90816.diff

1 Files Affected:

• (modified) llvm/include/llvm/Support/CommandLine.h (+1)

diff --git a/llvm/include/llvm/Support/CommandLine.h b/llvm/include/llvm/Support/CommandLine.h
index b035209406b680..8acdd5b12efaec 100644
--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -865,6 +865,7 @@ template <class DataType> class parser : public generic_parser_base {
 
     for (size_t i = 0, e = Values.size(); i != e; ++i)
       if (Values[i].Name == ArgVal) {
+        O.setValueStr(Values[i].Name);
         V = Values[i].V.getValue();
         return false;
       }

[igorkudrin]

Could you add a test?

[MaskRay]

Could you add a test?

Perhaps a unittest in llvm/unittests/Support/CommandLineTest.cpp

[bwendling]

Could you add a test?

Perhaps a unittest in llvm/unittests/Support/CommandLineTest.cpp

Sure. I'll do that soon.

[bwendling]

Done. (That was more work than it needed to be.) PTAL.

[igorkudrin]

Thanks for the test. However, it is still not clear to me what the motivation for the change was. Maybe similar changes should be made to the specializations of the cl::parser class? Should the change be reflected in the documentation?

[bwendling]

Thanks for the test. However, it is still not clear to me what the motivation for the change was. Maybe similar changes should be made to the specializations of the cl::parser class?

I'm doing this for an upcoming change where I want to know which register allocator is used (if it's not the "greedy" allocator, we won't support the change). I know it's a bit of a hack, but it's the best we can do without a rewrite of a large part of ISel and other places. (I can add you to the PR when I create it.)

As for other similar changes, it looks like all of the other fields are filled out correctly.

Should the change be reflected in the documentation?

Maybe? To be honest, I was surprised that it wasn't available in the situation I'm working with. In the case of the -regalloc=<name> flag, it doesn't have an enum, just its own parser that has the list of registered allocators in it. The "value" set (in the non-ValueStr field) is a pointer to the constructor for that register allocator, but we don't have access to the original <name> in the flag. That should be in the ValueStr field (if I understand correctly). So it's kinda fixing a deficiency IMO... If you want me to add it to the documentation, I'm fine with that. Just let me know which section is good. :-)

[bwendling]

Friendly ping.

[igorkudrin]

My understanding is that ValueStr is just an implementation detail to support the cl::value_desc modifier, as in

static cl::opt<std::string> OutputFilename("o", cl::desc("Output filename"), cl::value_desc("filename"));

which result in

> tool --help
  -o <filename>  - Output filename

It would not be correct to reuse the same member for another purpose.

[bwendling]

My understanding is that ValueStr is just an implementation detail to support the cl::value_desc modifier, as in

static cl::opt<std::string> OutputFilename("o", cl::desc("Output filename"), cl::value_desc("filename"));

which result in

> tool --help
  -o <filename>  - Output filename

It would not be correct to reuse the same member for another purpose.

Crumbs! Okay, I was mistaken. I think that Python calls this metavar, which I personally believe is clearer (but won't press the issue). Anyway, I'll close this PR. Thank you for the review.