Page MenuHomePhabricator

Initialize common options in `getRegisteredOptions`
ClosedPublic

Authored by zsrkmyn on Jul 19 2021, 7:38 PM.

Details

Summary

This allows users accessing options in libSupport before invoking
cl::ParseCommandLineOptions, and also matches the behavior before
D105959.

Diff Detail

Event Timeline

zsrkmyn created this revision.Jul 19 2021, 7:38 PM
zsrkmyn requested review of this revision.Jul 19 2021, 7:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 7:38 PM

I'm not sure if we need a test for it. I tried to add one but the test passed even without this patch, as those options were already initialized before running the test.

diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp
index a0352bc8a4c5..c18d34ab4323 100644
--- a/llvm/unittests/Support/CommandLineTest.cpp
+++ b/llvm/unittests/Support/CommandLineTest.cpp
@@ -89,6 +89,12 @@ public:
   ~StackSubCommand() { unregisterSubCommand(); }
 };
 
+TEST(CommandLineTest, RetrievePresetOptions) {
+  StringMap<cl::Option *> &Map =
+      cl::getRegisteredOptions(*cl::TopLevelSubCommand);
+
+  ASSERT_TRUE(Map.count("help") == 1) << "Could not get preset option `help`";
+}
 
 cl::OptionCategory TestCategory("Test Options", "Description");
 TEST(CommandLineTest, ModifyExisitingOption) {

I'm not sure if we need a test for it. I tried to add one but the test passed even without this patch, as those options were already initialized before running the test.

Seems nice to have a test for this...

One idea would be to split this test out of SupportTests to avoid being affected by other tests' running the initializer... assuming that's what's preventing the test from being useful?

Another idea would be to add a new llvm-clopt-test executable that exposes this codepath somehow. The tool could check if the first command-line argument is -register, and avoid parsing the command-line in that case.

Is there a way to clear all the registered options, prior to reregistering them again? Alternatively, would it be simple to add? I'm guessing even if it were, it might not solve our problems though, since the variables are local statics, so resetting wouldn't cause them to be reinstantiated...

I'm guessing even if it were, it might not solve our problems though, since the variables are local statics, so resetting wouldn't cause them to be reinstantiated...

I believe so. So I think I need to split the test from SupportTests as @dexonsmith suggested.

LGTM, thanks!
(better with a test as @dexonsmith suggested)

MaskRay requested changes to this revision.Jul 24 2021, 2:40 PM

(unittest request)

This revision now requires changes to proceed.Jul 24 2021, 2:40 PM
zsrkmyn updated this revision to Diff 361631.Jul 26 2021, 5:27 AM

Test added ;-)

A soft ping.

MaskRay added inline comments.Aug 1 2021, 8:53 PM
llvm/unittests/Support/CommandLineInit/CMakeLists.txt
2

This file looks a bit complex. Is all stuff here needed?

llvm/unittests/Support/CommandLineInit/CommandLineInitTest.cpp
32

State where this piece of code is copied from.

unittest/UnitTestMain/TestMain.cpp ?

zsrkmyn added inline comments.Aug 2 2021, 12:54 AM
llvm/unittests/Support/CommandLineInit/CMakeLists.txt
2

I believe so. Most part of the file was copied from add_unittest in llvm/cmake/modules/AddLLVM.cmake, except that gtest_main was excluded from target_link_libraries to prevent the test linking against TestMain.cpp.

llvm/unittests/Support/CommandLineInit/CommandLineInitTest.cpp
32

Yep. Should I add a comment in the source code?

zsrkmyn marked 2 inline comments as not done.Aug 2 2021, 1:01 AM
MaskRay accepted this revision.Aug 2 2021, 9:34 AM
MaskRay added inline comments.
llvm/unittests/Support/CommandLineInit/CMakeLists.txt
2

OK. Then state where the code is copied from.

llvm/unittests/Support/CommandLineInit/CommandLineInitTest.cpp
32

Yes

This revision is now accepted and ready to land.Aug 2 2021, 9:34 AM
zsrkmyn updated this revision to Diff 363609.Aug 2 2021, 6:21 PM
zsrkmyn added a comment.EditedAug 2 2021, 6:23 PM

Would you mind helping me commit it if there's no further comments? Many thanks!

llvm/unittests/Support/CommandLineInit/CMakeLists.txt
2

Done. Added at line 7.

llvm/unittests/Support/CommandLineInit/CommandLineInitTest.cpp
32

Done. Added at line 14.

This revision was automatically updated to reflect the committed changes.