CommandParser::LoopCall() changes

- Change function to be static, return a bool
- Do not filter duplicates when there are 2 lists (JOIN)
- Remove validation of 'extra' parameter, caller must pass either a valid index or -1

10 files changed, 59 insertions, 54 deletions

diff --git a/include/command_parse.h b/include/command_parse.h
index 4a88a8ec8..895fbb6e5 100644
--- a/include/command_parse.h
+++ b/include/command_parse.h
@@ -80,28 +80,40 @@ class CoreExport CommandParser
 	 */
 	bool IsValidCommand(const std::string &commandname, unsigned int pcnt, User * user);
 
-	/** LoopCall is used to call a command classes handler repeatedly based on the contents of a comma seperated list.
-	 * There are two overriden versions of this method, one of which takes two potential lists and the other takes one.
-	 * We need a version which takes two potential lists for JOIN, because a JOIN may contain two lists of items at once,
+	/** LoopCall is used to call a command handler repeatedly based on the contents of a comma seperated list.
+	 * There are two ways to call this method, either with one potential list or with two potential lists.
+	 * We need to handle two potential lists for JOIN, because a JOIN may contain two lists of items at once:
 	 * the channel names and their keys as follows:
 	 *
 	 * JOIN \#chan1,\#chan2,\#chan3 key1,,key3
 	 *
-	 * Therefore, we need to deal with both lists concurrently. The first instance of this method does that by creating
-	 * two instances of irc::commasepstream and reading them both together until the first runs out of tokens.
-	 * The second version is much simpler and just has the one stream to read, and is used in NAMES, WHOIS, PRIVMSG etc.
-	 * Both will only parse until they reach ServerInstance->Config->MaxTargets number of targets, to stop abuse via spam.
+	 * Therefore, we need to deal with both lists concurrently. If there are two lists then the method reads
+	 * them both together until the first runs out of tokens.
+	 * With one list it is much simpler, and is used in NAMES, WHOIS, PRIVMSG etc.
+	 *
+	 * If there is only one list and there are duplicates in it, then the command handler is only called for
+	 * unique items. Entries are compared using "irc comparision" (see irc::string).
+	 * If the usemax parameter is true (the default) the function only parses until it reaches
+	 * ServerInstance->Config->MaxTargets number of targets, to stop abuse via spam.
+	 *
+	 * If there are two lists and the second list runs out of tokens before the first list then parameters[extra]
+	 * will be an EMPTY string when Handle() is called for the remaining tokens in the first list, even if it is
+	 * in the middle of parameters[]! Moreover, empty tokens in the second list are allowed, and those will also
+	 * result in the appropiate entry being empty in parameters[].
+	 * This is different than what command handlers usually expect; the command parser only allows an empty param
+	 * as the last item in the vector.
 	 *
 	 * @param user The user who sent the command
 	 * @param CommandObj the command object to call for each parameter in the list
-	 * @param parameters Parameter list as an array of array of char (that's not a typo).
+	 * @param parameters Parameter list as a vector of strings
 	 * @param splithere The first parameter index to split as a comma seperated list
-	 * @param extra The second parameter index to split as a comma seperated list
-	 * @param usemax Limit the command to MaxTargets targets
-	 * @return This function will return 1 when there are no more parameters to process. When this occurs, its
-	 * caller should return without doing anything, otherwise it should continue into its main section of code.
+	 * @param extra The second parameter index to split as a comma seperated list, or -1 (the default) if there is only one list
+	 * @param usemax True to limit the command to MaxTargets targets (default), or false to process all tokens
+	 * @return This function returns true when it identified a list in the given parameter and finished calling the
+	 * command handler for each entry on the list. When this occurs, the caller should return without doing anything,
+	 * otherwise it should continue into its main section of code.
 	 */
-	int LoopCall(User* user, Command* CommandObj, const std::vector<std::string>& parameters, unsigned int splithere, int extra = -1, bool usemax = true);
+	static bool LoopCall(User* user, Command* CommandObj, const std::vector<std::string>& parameters, unsigned int splithere, int extra = -1, bool usemax = true);
 
 	/** Take a raw input buffer from a recvq, and process it on behalf of a user.
 	 * @param buffer The buffer line to process
diff --git a/src/command_parse.cpp b/src/command_parse.cpp
index e8d68c847..6331b5da4 100644
--- a/src/command_parse.cpp
+++ b/src/command_parse.cpp
@@ -44,67 +44,60 @@ int InspIRCd::PassCompare(Extensible* ex, const std::string &data, const std::st
 	return (data != input); // this seems back to front, but returns 0 if they *match*, 1 else
 }
 
-/* LoopCall is used to call a command classes handler repeatedly based on the contents of a comma seperated list.
- * There are two overriden versions of this method, one of which takes two potential lists and the other takes one.
- * We need a version which takes two potential lists for JOIN, because a JOIN may contain two lists of items at once,
- * the channel names and their keys as follows:
- * JOIN #chan1,#chan2,#chan3 key1,,key3
- * Therefore, we need to deal with both lists concurrently. The first instance of this method does that by creating
- * two instances of irc::commasepstream and reading them both together until the first runs out of tokens.
- * The second version is much simpler and just has the one stream to read, and is used in NAMES, WHOIS, PRIVMSG etc.
- * Both will only parse until they reach ServerInstance->Config->MaxTargets number of targets, to stop abuse via spam.
- */
-int CommandParser::LoopCall(User* user, Command* CommandObj, const std::vector<std::string>& parameters, unsigned int splithere, int extra, bool usemax)
+bool CommandParser::LoopCall(User* user, Command* CommandObj, const std::vector<std::string>& parameters, unsigned int splithere, int extra, bool usemax)
 {
 	if (splithere >= parameters.size())
-		return 0;
-
-	if (extra >= (signed)parameters.size())
-		extra = -1;
+		return false;
 
-	/* First check if we have more than one item in the list, if we don't we return zero here and the handler
+	/* First check if we have more than one item in the list, if we don't we return false here and the handler
 	 * which called us just carries on as it was.
 	 */
 	if (parameters[splithere].find(',') == std::string::npos)
-		return 0;
+		return false;
 
 	/** Some lame ircds will weed out dupes using some shitty O(n^2) algorithm.
 	 * By using std::set (thanks for the idea w00t) we can cut this down a ton.
 	 * ...VOOODOOOO!
+	 *
+	 * Only check for duplicates if there is one list (allow them in JOIN).
 	 */
 	std::set<irc::string> dupes;
+	bool check_dupes = (extra < 0);
 
-	/* Create two lists, one for channel names, one for keys
+	/* Create two sepstreams, if we have only one list, then initialize the second sepstream with
+	 * an empty string. The second parameter of the constructor of the sepstream tells whether
+	 * or not to allow empty tokens.
+	 * We allow empty keys, so "JOIN #a,#b ,bkey" will be interpreted as "JOIN #a", "JOIN #b bkey"
 	 */
 	irc::commasepstream items1(parameters[splithere]);
-	irc::commasepstream items2(extra >= 0 ? parameters[extra] : "");
-	std::string extrastuff;
+	irc::commasepstream items2(extra >= 0 ? parameters[extra] : "", true);
 	std::string item;
 	unsigned int max = 0;
 
-	/* Attempt to iterate these lists and call the command objech
-	 * which called us, for every parameter pair until there are
-	 * no more left to parse.
+	/* Attempt to iterate these lists and call the command handler
+	 * for every parameter or parameter pair until there are no more
+	 * left to parse.
 	 */
 	while (items1.GetToken(item) && (!usemax || max++ < ServerInstance->Config->MaxTargets))
 	{
-		if (dupes.find(item.c_str()) == dupes.end())
+		if ((!check_dupes) || (dupes.insert(item.c_str()).second))
 		{
 			std::vector<std::string> new_parameters(parameters);
-
-			if (!items2.GetToken(extrastuff))
-				extrastuff.clear();
-
 			new_parameters[splithere] = item;
+
 			if (extra >= 0)
-				new_parameters[extra] = extrastuff;
+			{
+				// If we have two lists then get the next item from the second list.
+				// In case it runs out of elements then 'item' will be an empty string.
+				items2.GetToken(item);
+				new_parameters[extra] = item;
+			}
 
 			CommandObj->Handle(new_parameters, user);
-
-			dupes.insert(item.c_str());
 		}
 	}
-	return 1;
+
+	return true;
 }
 
 bool CommandParser::IsValidCommand(const std::string &commandname, unsigned int pcnt, User * user)
diff --git a/src/commands/cmd_join.cpp b/src/commands/cmd_join.cpp
index a88509bc2..9e2678b5d 100644
--- a/src/commands/cmd_join.cpp
+++ b/src/commands/cmd_join.cpp
@@ -52,7 +52,7 @@ CmdResult CommandJoin::HandleLocal(const std::vector<std::string>& parameters, L
 {
 	if (parameters.size() > 1)
 	{
-		if (ServerInstance->Parser->LoopCall(user, this, parameters, 0, 1, false))
+		if (CommandParser::LoopCall(user, this, parameters, 0, 1, false))
 			return CMD_SUCCESS;
 
 		if (ServerInstance->IsChannel(parameters[0]))
@@ -63,7 +63,7 @@ CmdResult CommandJoin::HandleLocal(const std::vector<std::string>& parameters, L
 	}
 	else
 	{
-		if (ServerInstance->Parser->LoopCall(user, this, parameters, 0, -1, false))
+		if (CommandParser::LoopCall(user, this, parameters, 0, -1, false))
 			return CMD_SUCCESS;
 
 		if (ServerInstance->IsChannel(parameters[0]))
diff --git a/src/commands/cmd_kick.cpp b/src/commands/cmd_kick.cpp
index 016a14b2e..825cfebcb 100644
--- a/src/commands/cmd_kick.cpp
+++ b/src/commands/cmd_kick.cpp
@@ -48,7 +48,7 @@ CmdResult CommandKick::Handle (const std::vector<std::string>& parameters, User
 	Channel* c = ServerInstance->FindChan(parameters[0]);
 	User* u;
 
-	if (ServerInstance->Parser->LoopCall(user, this, parameters, 1))
+	if (CommandParser::LoopCall(user, this, parameters, 1))
 		return CMD_SUCCESS;
 
 	if (IS_LOCAL(user))
diff --git a/src/commands/cmd_kill.cpp b/src/commands/cmd_kill.cpp
index 6bf657115..04a59f60a 100644
--- a/src/commands/cmd_kill.cpp
+++ b/src/commands/cmd_kill.cpp
@@ -57,7 +57,7 @@ class CommandKill : public Command
 CmdResult CommandKill::Handle (const std::vector<std::string>& parameters, User *user)
 {
 	/* Allow comma seperated lists of users for /KILL (thanks w00t) */
-	if (ServerInstance->Parser->LoopCall(user, this, parameters, 0))
+	if (CommandParser::LoopCall(user, this, parameters, 0))
 		return CMD_SUCCESS;
 
 	User *u = ServerInstance->FindNick(parameters[0]);
diff --git a/src/commands/cmd_names.cpp b/src/commands/cmd_names.cpp
index 0c06b636f..1f0de91f1 100644
--- a/src/commands/cmd_names.cpp
+++ b/src/commands/cmd_names.cpp
@@ -52,7 +52,7 @@ CmdResult CommandNames::Handle (const std::vector<std::string>& parameters, User
 		return CMD_SUCCESS;
 	}
 
-	if (ServerInstance->Parser->LoopCall(user, this, parameters, 0))
+	if (CommandParser::LoopCall(user, this, parameters, 0))
 		return CMD_SUCCESS;
 
 	c = ServerInstance->FindChan(parameters[0]);
diff --git a/src/commands/cmd_part.cpp b/src/commands/cmd_part.cpp
index aadb42d90..9b8d1d717 100644
--- a/src/commands/cmd_part.cpp
+++ b/src/commands/cmd_part.cpp
@@ -57,7 +57,7 @@ CmdResult CommandPart::Handle (const std::vector<std::string>& parameters, User
 			reason = parameters[1];
 	}
 
-	if (ServerInstance->Parser->LoopCall(user, this, parameters, 0))
+	if (CommandParser::LoopCall(user, this, parameters, 0))
 		return CMD_SUCCESS;
 
 	Channel* c = ServerInstance->FindChan(parameters[0]);
diff --git a/src/commands/cmd_privmsg.cpp b/src/commands/cmd_privmsg.cpp
index eb9468bb9..7de3bf924 100644
--- a/src/commands/cmd_privmsg.cpp
+++ b/src/commands/cmd_privmsg.cpp
@@ -62,7 +62,7 @@ CmdResult MessageCommandBase::HandleMessage(const std::vector<std::string>& para
 	if (localuser)
 		localuser->idle_lastmsg = ServerInstance->Time();
 
-	if (ServerInstance->Parser->LoopCall(user, this, parameters, 0))
+	if (CommandParser::LoopCall(user, this, parameters, 0))
 		return CMD_SUCCESS;
 
 	if (parameters[0][0] == '$')
diff --git a/src/commands/cmd_whois.cpp b/src/commands/cmd_whois.cpp
index de3d71152..9048184f6 100644
--- a/src/commands/cmd_whois.cpp
+++ b/src/commands/cmd_whois.cpp
@@ -195,7 +195,7 @@ CmdResult CommandWhois::HandleLocal(const std::vector<std::string>& parameters,
 	int userindex = 0;
 	unsigned long idle = 0, signon = 0;
 
-	if (ServerInstance->Parser->LoopCall(user, this, parameters, 0))
+	if (CommandParser::LoopCall(user, this, parameters, 0))
 		return CMD_SUCCESS;
 
 	/*
diff --git a/src/modules/m_callerid.cpp b/src/modules/m_callerid.cpp
index 9cc9aaf5e..0fffe4061 100644
--- a/src/modules/m_callerid.cpp
+++ b/src/modules/m_callerid.cpp
@@ -189,7 +189,7 @@ public:
 	 */
 	CmdResult Handle(const std::vector<std::string> &parameters, User* user)
 	{
-		if (ServerInstance->Parser->LoopCall(user, this, parameters, 0))
+		if (CommandParser::LoopCall(user, this, parameters, 0))
 			return CMD_SUCCESS;
 		/* Even if callerid mode is not set, we let them manage their ACCEPT list so that if they go +g they can
 		 * have a list already setup. */