Fix default -m delimiter for note ACL

[ankor2023]

This PR fixes a bug where acl note would split values by a comma
even if the -m option was not provided.

Current behavior:
If -m is omitted, AnnotationCheck defaults to a comma (,),
causing tokens to be split unexpectedly.

New behavior:
When -m is not used, no splitting occurs (the entire value is
treated as a single token).

[yadij]

Shortened the title and description to fit within Anubis enforced bounds. Dropped the text listing what the patch contains (redundant), and the reference to original discussion (URL too long for commit message).
Original thread reference is https://ml-archives.squid-cache.org/squid-dev/2026-April/010017.html

[yadij]

Some documentation polish for what is there now.

[yadij]

Suggested change

	const Value *value() const { return enabled() ? &explicitOrDefaultValue : nullptr; }; ///< accessor
	/// \retval nullptr Do not use any value for this option.
	const Value *value() const { return enabled() ? &explicitOrDefaultValue : nullptr; };

[rousskov]

I am against the suggestion in this change request because it contains arguably misleading caller instructions (where no instructions are necessary at all) and because the suggested wording implies that a caller has access to multiple option values. I do agree that "accessor" is also problematic and posted a change request with a specific replacement suggestion.

[yadij]

There is no change from the implications created by the member naming ( "AorB" ) except to clarify that hidden nullptr semantic meaning. I am open to alternatives that clarify the three semantic meanings better.

[ankor2023]

@yadij

There is no change from the implications created by the member naming ( "AorB" ) except to clarify that hidden nullptr semantic meaning. I am open to alternatives that clarify the three semantic meanings better.

Alex suggested the following comment for this method:

/// option value (if the option was enabled) or nil (otherwise)
const Value *value() const { return enabled() ?  &explicitOrDefaultValue : nullptr; }

and for the variable description:

/// When 'valued' is true, this is an explicitly set value.
/// Otherwise, this is the default value.
Value explicitOrDefaultValue;

In my opinion, this covers the three possible semantic meanings quite well.

[rousskov]

This PR needs more work. I need more time to flag and/or fix other issues, but I am posting this partial review to facilitate progress and in hope to reduce churn related to already posted change requests.

[rousskov]

I am against the suggestion in this change request because it contains arguably misleading caller instructions (where no instructions are necessary at all) and because the suggested wording implies that a caller has access to multiple option values. I do agree that "accessor" is also problematic and posted a change request with a specific replacement suggestion.

[rousskov]

If you are adding this if statement "just in case", please do not add it. It is the caller's responsibility to check that the value exists as it is not possible to print a non-existing value correctly. This method can (and probably should) assert (using Assure()) that the value exists.

Otherwise, please discuss what caller(s) prompted you to add this if statement.

[ankor2023]

@rousskov

If you are adding this if statement "just in case", please do not add it. It is the caller's responsibility to check that the value exists as it is not possible to print a non-existing value correctly.

I assumed this method could be used for debugging purposes. In this case, as I understand it, the calling procedure might not know whether the internal variable has been set.

This method can (and probably should) assert (using Assure()) that the value exists.

Refactor it to the following, then?

template <>
inline void
TypedOption<CharacterSetOptionValue>::printValue(std::ostream &os) const
{
    Assure(recipient_->value());
    recipient_->value()->printChars(os); // TODO: Quote if needed.
}

[rousskov]

I assumed this method could be used for debugging purposes.

I do not think that is a valid assumption in this case. Debugging code should use higher-level methods that handle non-existent values correctly.

Refactor it to the following, then?

If you really insist. FWIW, my recommendation for this method in this PR is to just add () to get this code to compile. This PR is not about improving other aspects of this method, and the proposed assertion is not related to the primary PR changes.