Copilot commented on code in PR #433:
URL: https://github.com/apache/commons-cli/pull/433#discussion_r3645443917
##########
src/main/java/org/apache/commons/cli/DefaultParser.java:
##########
@@ -518,16 +518,17 @@ private void handleProperties(final Properties
properties) throws ParseException
if (!cmd.hasOption(option) && !selected) {
// get the value from the properties
final String value = properties.getProperty(option);
-
- if (opt.hasArg()) {
- if (opt.isValuesEmpty()) {
-
opt.processValue(stripLeadingAndTrailingQuotesDefaultOff(value));
+ // the Options belong to the caller and outlive this parse, so
hold the value on a copy
+ final Option copy = (Option) opt.clone();
+ if (copy.hasArg()) {
+ if (copy.isValuesEmpty()) {
+
copy.processValue(stripLeadingAndTrailingQuotesDefaultOff(value));
Review Comment:
Similar to `Parser.processProperties`, this clones `opt` before determining
whether a no-arg option will be ignored (when the property value is not
yes/true/1 and the code hits `continue`). Consider deferring cloning for the
no-arg path until after the truthy check so you avoid creating a copy when the
option won’t be added.
##########
src/main/java/org/apache/commons/cli/Parser.java:
##########
@@ -284,10 +284,12 @@ protected void processProperties(final Properties
properties) throws ParseExcept
if (!cmd.hasOption(option) && !selected) {
// get the value from the properties instance
final String value = properties.getProperty(option);
- if (opt.hasArg()) {
- if (opt.isValuesEmpty()) {
+ // the Options belong to the caller and outlive this parse, so
hold the value on a copy
+ final Option copy = (Option) opt.clone();
+ if (copy.hasArg()) {
+ if (copy.isValuesEmpty()) {
Review Comment:
The clone is created unconditionally for each eligible property entry, even
in the no-arg (`else`) case where the code may `continue` (when the property
value is not truthy) and never uses the cloned instance. Consider restructuring
so the clone is only created when the option will actually be added to the
`CommandLine` (e.g., defer cloning until after the falsy check for flag
options). This reduces allocations when properties contain disabled flags.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
