Skip to content

Commit 39f8071

Browse files
johan-andruejoljohan-andruejol
committed
ENH: ParameterSerializer: Fix multiple parameters handling for deserialization
The code wasn't handling multiple parameters correctly and crashing when trying to deserialize arrays of arrays. This commit addresses that. Arguments using "multiple=true" can either be flagged or positional. - When they are positional, they must be the last argument of the CLI. In this case, the infrastructure already handles this. - When they are flagged, we need to detect that the argument allows for multiple flag/value pairs. To do so, we introduce the map called deserializedMultipleArgsMap. It stores a map of flag to vector of values. Json can populate that map based on the values it reads. Otherwise the command line parsing will populate this map. When an argument using "multiple=true" is present in the JSON and in the command line, simirarly than with other arguments, the values of the JSON are ignored. Testing for all the types that support the multiple argument is added.
1 parent 6e34287 commit 39f8071

File tree

5 files changed

+439
-24
lines changed

5 files changed

+439
-24
lines changed

‎GenerateCLP/GenerateCLP.cxx‎

Lines changed: 134 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -560,6 +560,39 @@ void GenerateDeSerialization( std::ostream & sout,
560560
| " deserializedVectorFlaggedArgs.push_back(\"" + flag + "\");"
561561
| " }";
562562
}
563+
else if (IsVectorOfVectors(*paramIt) || paramIt->GetMultiple() == "true")
564+
{
565+
w | " Json::Value param = parameters[\"" + groupLabel + "\"][\"" + parameterName + "\"];"
566+
| " if(param.size() > 0)"
567+
| " {"
568+
| " std::vector<std::string> values;"
569+
| " for (unsigned int i = 0; i < param.size(); ++i)"
570+
| " {";
571+
if (IsVectorOfVectors(*paramIt))
572+
{
573+
w | " std::string value = \"\";"
574+
| " for (unsigned int j = 0; j < param[i].size(); ++j)"
575+
| " {"
576+
| " value += param[i][j].asString();"
577+
| " if (j < param[i].size() - 1)"
578+
| " {"
579+
| " value += \", \";"
580+
| " }"
581+
| " }"
582+
| " values.push_back(value);";
583+
}
584+
else
585+
{
586+
w | " values.push_back(param[i].asString());";
587+
}
588+
589+
w | " }"
590+
| " if (!values.empty())"
591+
| " {"
592+
| " deserializedMultipleArgsMap[\"" + flag + "\"] = values;"
593+
| " }"
594+
| " }";
595+
}
563596
else
564597
{
565598
w | " Json::Value param = parameters[\"" + groupLabel + "\"][\"" + parameterName + "\"];"
@@ -570,7 +603,6 @@ void GenerateDeSerialization( std::ostream & sout,
570603
| " }"
571604
| " else if(param.size() > 0)"
572605
| " {"
573-
| " deserializedVectorFlaggedArgs.push_back(\"" + flag + "\");"
574606
| " std::string value = \"\";"
575607
| " for (unsigned int i = 0; i < param.size(); ++i)"
576608
| " {"
@@ -580,13 +612,28 @@ void GenerateDeSerialization( std::ostream & sout,
580612
| " value += \", \";"
581613
| " }"
582614
| " }"
615+
| " deserializedVectorFlaggedArgs.push_back(\"" + flag + "\");"
583616
| " deserializedVectorFlaggedArgs.push_back(value);"
584617
| " }";
585618
}
586619
}
587620
else
588621
{
589-
w | " deserializedVectorPositionalArgs.push_back(parameters[\"" + groupLabel + "\"][\"" + parameterName + "\"].asString());";
622+
if (paramIt->GetMultiple() == "true")
623+
{
624+
w | " Json::Value param = parameters[\"" + groupLabel + "\"][\"" + parameterName + "\"]"
625+
| " if(param.size() > 0)"
626+
| " {"
627+
| " for (unsigned int i = 0; i < param.size(); ++i)"
628+
| " {"
629+
| " deserializedVectorPositionalArgs.push_back(param[i].asString());"
630+
| " }"
631+
| " }";
632+
}
633+
else
634+
{
635+
w | " deserializedVectorPositionalArgs.push_back(parameters[\"" + groupLabel + "\"][\"" + parameterName + "\"].asString());";
636+
}
590637
}
591638
w | " }";
592639
}
@@ -1045,6 +1092,9 @@ void GenerateDeclare(std::ostream &sout, ModuleDescription &module)
10451092
sout << " /* that are then compiled with the command line. */" << EOL << std::endl;
10461093
sout << " std::vector< std::string > deserializedVectorFlaggedArgs;" << EOL << std::endl;
10471094
sout << " std::vector< std::string > deserializedVectorPositionalArgs;" << EOL << std::endl;
1095+
sout << " /* This map is used to store the JSON deserialized value of multiple args*/" << EOL << std::endl;
1096+
sout << " /* where the key is the argument flag and the value the values of each arg. */" << EOL << std::endl;
1097+
sout << " std::map< std::string, std::vector<std::string> > deserializedMultipleArgsMap;" << EOL << std::endl;
10481098
sout << EOL << std::endl;
10491099
sout << " /* This vector is used to look up if a flag requires an argument after it. */" << EOL << std::endl;
10501100
sout << " /* This is used to differentiate between: */" << EOL << std::endl;
@@ -1074,6 +1124,29 @@ void GenerateDeclare(std::ostream &sout, ModuleDescription &module)
10741124
}
10751125
}
10761126
}
1127+
sout << " /* This map use is twofold: */" << EOL << std::endl;
1128+
sout << " /* - to find whether a flag is multiple */" << EOL << std::endl;
1129+
sout << " /* - to know if we need to reset the multiple arg value because it was */" << EOL << std::endl;
1130+
sout << " /* in the JSON and it's also in the command line. */" << EOL << std::endl;
1131+
sout << " std::map<std::string, bool> multipleFlags;" << EOL << std::endl;
1132+
for (git = module.GetParameterGroups().begin();
1133+
git != module.GetParameterGroups().end();
1134+
++git)
1135+
{
1136+
for (pit = git->GetParameters().begin();
1137+
pit != git->GetParameters().end();
1138+
++pit)
1139+
{
1140+
if (pit->GetMultiple() == "true" && !pit->GetFlag().empty())
1141+
{
1142+
sout << " multipleFlags[\"-" << pit->GetFlag() << "\"] = false;" << EOL << std::endl;
1143+
}
1144+
if (pit->GetMultiple() == "true" && !pit->GetLongFlag().empty())
1145+
{
1146+
sout << " multipleFlags[\"--" << pit->GetLongFlag() << "\"] = false;" << EOL << std::endl;
1147+
}
1148+
}
1149+
}
10771150

10781151
// First pass generates argument declarations
10791152
for (git = module.GetParameterGroups().begin();
@@ -1523,6 +1596,7 @@ void GenerateTCLAPParse(std::ostream &sout, ModuleDescription &module)
15231596
sout << " std::map<std::string,std::string>::iterator ait;" << EOL << std::endl;
15241597
sout << " std::map<std::string,std::string>::iterator dait;" << EOL << std::endl;
15251598
sout << " std::vector<std::string>::iterator dvOptionnalArgsIt = deserializedVectorFlaggedArgs.begin();" << EOL << std::endl;
1599+
sout << " std::map<std::string, std::vector<std::string> >::iterator dvMultipleArgsIt;" << EOL << std::endl;
15261600
sout << " size_t noFlagCounter = 0;" << EOL << std::endl;
15271601
sout << " size_t ac = 1;" << EOL << std::endl;
15281602
sout << EOL << std::endl;
@@ -1571,48 +1645,75 @@ void GenerateTCLAPParse(std::ostream &sout, ModuleDescription &module)
15711645
sout << " flag = (*dait).second;" << EOL << std::endl;
15721646
sout << " }" << EOL << std::endl;
15731647
sout << " }" << EOL << std::endl;
1574-
// Compiling: 4 cases
1648+
sout << " bool isMultiple = multipleFlags.find(flag) != multipleFlags.end();" << EOL << std::endl;
15751649
sout << " bool isBoolean = std::find(nonbooleanFlags.begin(), nonbooleanFlags.end(), flag) == nonbooleanFlags.end();" << EOL << std::endl;
15761650
sout << " dvOptionnalArgsIt = std::find(deserializedVectorFlaggedArgs.begin(), deserializedVectorFlaggedArgs.end(), flag);" << EOL << std::endl;
1577-
sout << " /*Ignore if boolean and already present*/" << EOL << std::endl;
1578-
// Boolean && already present -> Do nothing
1579-
sout << " if (isBoolean && dvOptionnalArgsIt != deserializedVectorFlaggedArgs.end())" << EOL << std::endl;
1651+
sout << " bool isPresent = dvOptionnalArgsIt != deserializedVectorFlaggedArgs.end();" << EOL << std::endl;
1652+
// Compiling: 3 booleans => 8 cases
1653+
// But isBoolean && isMultiple is impossible, so a total of => 6 cases
1654+
sout << " if (isBoolean)" << EOL << std::endl;
1655+
// Boolean cases:
15801656
sout << " {" << EOL << std::endl;
1657+
sout << " /*Ignore if boolean and already present*/" << EOL << std::endl;
1658+
sout << " /*Otherwise add it*/" << EOL << std::endl;
1659+
sout << " if (!isPresent)" << EOL << std::endl;
1660+
// Already present -> Do nothing
1661+
// Not present -> Add the flag
1662+
sout << " {" << EOL << std::endl;
1663+
sout << " deserializedVectorFlaggedArgs.push_back(flag);" << EOL << std::endl;
1664+
sout << " }" << EOL << std::endl;
15811665
sout << " ++ac;" << EOL << std::endl;
15821666
sout << " }" << EOL << std::endl;
1583-
sout << " /*If not boolean and already present, update the flag value*/" << EOL << std::endl;
1584-
// NOT Boolean && already present -> Replace the flag value
1585-
sout << " else if (!isBoolean && dvOptionnalArgsIt != deserializedVectorFlaggedArgs.end())" << EOL << std::endl;
1667+
sout << " else if (isMultiple)" << EOL << std::endl;
15861668
sout << " {" << EOL << std::endl;
1669+
// Multiple cases:
1670+
sout << " dvMultipleArgsIt = deserializedMultipleArgsMap.find(flag);" << EOL << std::endl;
1671+
sout << " bool isPresent = dvMultipleArgsIt != deserializedMultipleArgsMap.end();" << EOL << std::endl;
1672+
sout << " /*Ignore if boolean and already present*/" << EOL << std::endl;
1673+
sout << " /*Reset/Add the value if first deserialize or not present*/" << EOL << std::endl;
1674+
sout << " if (!isPresent || !multipleFlags[flag])" << EOL << std::endl;
1675+
// We need to reset/add the value if:
1676+
// - Not present
1677+
// - Was deserialized and it's now found in the command line
1678+
sout << " {" << EOL << std::endl;
1679+
sout << " deserializedMultipleArgsMap[flag] = std::vector<std::string>();" << EOL << std::endl;
1680+
sout << " multipleFlags[flag] = true;" << EOL << std::endl;
1681+
sout << " }" << EOL << std::endl;
1682+
// In any case, add the value and move on
15871683
sout << " ++ac;" << EOL << std::endl;
15881684
sout << " std::string value = \"\";" << EOL << std::endl;
15891685
sout << " if (ac < static_cast<size_t>(argc))" << EOL << std::endl;
15901686
sout << " {" << EOL << std::endl;
15911687
sout << " value = argv[ac];" << EOL << std::endl;
15921688
sout << " ++ac;" << EOL << std::endl;
15931689
sout << " }" << EOL << std::endl;
1594-
sout << " *(++dvOptionnalArgsIt) = value;" << EOL << std::endl;
1690+
sout << " deserializedMultipleArgsMap[flag].push_back(value);" << EOL << std::endl;
15951691
sout << " }" << EOL << std::endl;
1596-
sout << " /*If boolean and not present, just add it*/" << EOL << std::endl;
1597-
// Boolean && NOT present -> Add the flag
1598-
sout << " else if (isBoolean && dvOptionnalArgsIt == deserializedVectorFlaggedArgs.end())" << EOL << std::endl;
1599-
sout << " {" << EOL << std::endl;
1600-
sout << " deserializedVectorFlaggedArgs.push_back(flag);" << EOL << std::endl;
1601-
sout << " ++ac;" << EOL << std::endl;
1602-
sout << " }" << EOL << std::endl;
1603-
sout << " /*If not boolean and not present, just add it and add value*/" << EOL << std::endl;
1604-
// NOT Boolean && NOT present -> Add the flag, add the value
1605-
sout << " else if (!isBoolean && dvOptionnalArgsIt == deserializedVectorFlaggedArgs.end())" << EOL << std::endl;
1692+
sout << " else" << EOL << std::endl;
1693+
// Other cases:
16061694
sout << " {" << EOL << std::endl;
1607-
sout << " deserializedVectorFlaggedArgs.push_back(flag);" << EOL << std::endl;
1695+
sout << " /*Add the flag and if needed*/" << EOL << std::endl;
1696+
sout << " if (!isPresent)" << EOL << std::endl;
1697+
sout << " {" << EOL << std::endl;
1698+
// Not here -> Add flag
1699+
sout << " deserializedVectorFlaggedArgs.push_back(flag);" << EOL << std::endl;
1700+
sout << " }" << EOL << std::endl;
16081701
sout << " ++ac;" << EOL << std::endl;
16091702
sout << " std::string value = \"\";" << EOL << std::endl;
16101703
sout << " if (ac < static_cast<size_t>(argc))" << EOL << std::endl;
16111704
sout << " {" << EOL << std::endl;
1612-
sout << " value = argv[ac]; "<< EOL << std::endl;
1705+
sout << " value = argv[ac];" << EOL << std::endl;
16131706
sout << " ++ac;" << EOL << std::endl;
16141707
sout << " }" << EOL << std::endl;
1615-
sout << " deserializedVectorFlaggedArgs.push_back(value); "<< EOL << std::endl;
1708+
// In any case add/set the value and move on
1709+
sout << " if (!isPresent)" << EOL << std::endl;
1710+
sout << " {" << EOL << std::endl;
1711+
sout << " deserializedVectorFlaggedArgs.push_back(value); "<< EOL << std::endl;
1712+
sout << " }" << EOL << std::endl;
1713+
sout << " else" << EOL << std::endl;
1714+
sout << " {" << EOL << std::endl;
1715+
sout << " *(++dvOptionnalArgsIt) = value;" << EOL << std::endl;
1716+
sout << " }" << EOL << std::endl;
16161717
sout << " }" << EOL << std::endl;
16171718
sout << " }" << EOL << std::endl;
16181719
sout << " /* short flag case where multiple flags are given at once */" << EOL << std::endl;
@@ -1671,8 +1772,17 @@ void GenerateTCLAPParse(std::ostream &sout, ModuleDescription &module)
16711772
sout << " std::vector<std::string> argvVector;" << EOL << std::endl;
16721773
sout << " argvVector.push_back(argv[0]);" << EOL << std::endl;
16731774
sout << " argvVector.insert(argvVector.end(), deserializedVectorFlaggedArgs.begin(), deserializedVectorFlaggedArgs.end());" << EOL << std::endl;
1775+
sout << " std::map<std::string, std::vector<std::string> >::iterator mavit;" << EOL << std::endl;
1776+
sout << " for (mavit = deserializedMultipleArgsMap.begin(); mavit != deserializedMultipleArgsMap.end(); ++mavit)" << EOL << std::endl;
1777+
sout << " {" << EOL << std::endl;
1778+
sout << " for (size_t i = 0; i < mavit->second.size(); ++i)" << EOL << std::endl;
1779+
sout << " {" << EOL << std::endl;
1780+
sout << " argvVector.push_back(mavit->first);" << EOL << std::endl;
1781+
sout << " argvVector.push_back(mavit->second.at(i));" << EOL << std::endl;
1782+
sout << " }" << EOL << std::endl;
1783+
sout << " }" << EOL << std::endl;
16741784
sout << " argvVector.insert(argvVector.end(), deserializedVectorPositionalArgs.begin(), deserializedVectorPositionalArgs.end());" << EOL << std::endl;
1675-
1785+
sout << EOL << std::endl;
16761786
sout << " /* Remap args to a structure that CmdLine::parse() can understand*/" << EOL << std::endl;
16771787
sout << " std::vector<char*> vargs;" << EOL << std::endl;
16781788
sout << " for (ac = 0; ac < argvVector.size(); ++ac)" << EOL << std::endl;

‎GenerateCLP/Testing/CLPTestMultiple/CLPTestMultiple.cxx‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
2+
#include "CLPTestMultipleCLP.h"
3+
4+
#include <vector>
5+
6+
template<class T>
7+
bool Compare(T* expectedValues,
8+
unsigned int expectedNumberOfValues,
9+
std::vector<T>& values
10+
)
11+
{
12+
bool success = values.size() == expectedNumberOfValues;
13+
if (!success)
14+
{
15+
std::cerr << "Wrong number of values, got: " << values.size()
16+
<< " expected: " << expectedNumberOfValues << std::endl;
17+
return false;
18+
}
19+
20+
for (size_t i = 0; i < expectedNumberOfValues; ++i)
21+
{
22+
success &= values[i] == expectedValues[i];
23+
if (!success)
24+
{
25+
std::cerr << "Wrong values, got: " << values[i]
26+
<< " expected: " << expectedValues[i] << std::endl;
27+
}
28+
}
29+
return success;
30+
}
31+
32+
//-----------------------------------------------------------------------------
33+
int main(int argc, char * argv[])
34+
{
35+
PARSE_ARGS;
36+
37+
const unsigned int numberOfExpectedValues = 6;
38+
int expectedValues[numberOfExpectedValues] = {1, 2, 3, 4, 5, 6};
39+
int negativeExpectedValues[numberOfExpectedValues] = {-1, -2, -3, -4, -5, -6};
40+
std::string stringExpectedValues[numberOfExpectedValues] = {"1", "2", "3", "4", "5", "6"};
41+
std::string negativeStringExpectedValues[numberOfExpectedValues] = {"-1", "-2", "-3", "-4", "-5", "-6"};
42+
43+
int floatVectorSizes[3] = {1, 3, 2};
44+
float floatExpectedValues[3][3] = {
45+
{1, 0, 0},
46+
{2, 3, 4},
47+
{5, 6, 0}
48+
};
49+
int negativeFloatVectorSizes[3] = {2, 1, 3};
50+
float negativeFloatExpectedValues[3][3] = {
51+
{-1, -2, 0},
52+
{-3, 0, 0},
53+
{-4, -5, -6}
54+
};
55+
56+
bool success = false;
57+
if (IntList.size() > 0)
58+
{
59+
success = Compare<int>(
60+
TestCommandLineOverWrite ? negativeExpectedValues : expectedValues,
61+
numberOfExpectedValues,
62+
IntList);
63+
}
64+
else if (FileList.size() > 0)
65+
{
66+
success = Compare<std::string>(
67+
TestCommandLineOverWrite ? negativeStringExpectedValues : stringExpectedValues,
68+
numberOfExpectedValues,
69+
FileList);
70+
}
71+
else if (DirList.size() > 0)
72+
{
73+
success = Compare<std::string>(
74+
TestCommandLineOverWrite ? negativeStringExpectedValues : stringExpectedValues,
75+
numberOfExpectedValues,
76+
DirList);
77+
}
78+
else if (ImageList.size() > 0)
79+
{
80+
success = Compare<std::string>(
81+
TestCommandLineOverWrite ? negativeStringExpectedValues : stringExpectedValues,
82+
numberOfExpectedValues,
83+
ImageList);
84+
}
85+
else if (GeometryList.size() > 0)
86+
{
87+
success = Compare<std::string>(
88+
TestCommandLineOverWrite ? negativeStringExpectedValues : stringExpectedValues,
89+
numberOfExpectedValues,
90+
GeometryList);
91+
}
92+
else if (PointList.size() > 0)
93+
{
94+
success = PointList.size() == 3;
95+
if (!success)
96+
{
97+
std::cerr << "Wrong number of values, got: " << PointList.size()
98+
<< " expected: " << 3 << std::endl;
99+
return EXIT_FAILURE;
100+
}
101+
for (size_t i = 0; i < 3; ++i)
102+
{
103+
success &=
104+
Compare<float>(
105+
TestCommandLineOverWrite ? negativeFloatExpectedValues[i] : floatExpectedValues[i],
106+
TestCommandLineOverWrite ? negativeFloatVectorSizes[i] : floatVectorSizes[i],
107+
PointList[i]);
108+
}
109+
}
110+
else if (PointFileList.size() > 0)
111+
{
112+
success = Compare<std::string>(
113+
TestCommandLineOverWrite ? negativeStringExpectedValues : stringExpectedValues,
114+
numberOfExpectedValues,
115+
PointFileList);
116+
}
117+
else if (RegionList.size() > 0)
118+
{
119+
success = RegionList.size() == 3;
120+
if (!success)
121+
{
122+
std::cerr << "Wrong number of values, got: " << RegionList.size()
123+
<< " expected: " << 3 << std::endl;
124+
return EXIT_FAILURE;
125+
}
126+
for (size_t i = 0; i < 3; ++i)
127+
{
128+
success &=
129+
Compare<float>(
130+
TestCommandLineOverWrite ? negativeFloatExpectedValues[i] : floatExpectedValues[i],
131+
TestCommandLineOverWrite ? negativeFloatVectorSizes[i] : floatVectorSizes[i],
132+
RegionList[i]);
133+
}
134+
}
135+
136+
if (success)
137+
{
138+
return EXIT_SUCCESS;
139+
}
140+
std::cerr << "Fail" << std::endl;
141+
return EXIT_FAILURE;
142+
}

0 commit comments

Comments
 (0)