Page MenuHomeWildfire Games
Differential D4677 Diff 20618 source/ps/GameSetup/CmdLineArgs.cpp

Changeset View

Standalone View

View Options

source/ps/GameSetup/CmdLineArgs.cpp

/* Copyright (C) 2021 Wildfire Games. /* Copyright (C) 2022 Wildfire Games.
* This file is part of 0 A.D. * This file is part of 0 A.D.
* *
* 0 A.D. is free software: you can redistribute it and/or modify * 0 A.D. is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by * it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or * the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version. * (at your option) any later version.
* *
* 0 A.D. is distributed in the hope that it will be useful, * 0 A.D. is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of * but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details. * GNU General Public License for more details.
* *
* You should have received a copy of the GNU General Public License * You should have received a copy of the GNU General Public License
* along with 0 A.D. If not, see <http://www.gnu.org/licenses/>. * along with 0 A.D. If not, see <http://www.gnu.org/licenses/>.
*/ */
#include "precompiled.h" #include "precompiled.h"
#include "CmdLineArgs.h" #include "CmdLineArgs.h"
#include "lib/sysdep/sysdep.h" #include "lib/sysdep/sysdep.h"
#include <algorithm>
#include <iterator>
#include <string_view>
CmdLineArgs g_CmdLineArgs; CmdLineArgs g_CmdLineArgs;
namespace namespace
{ {
// Simple matcher for elements of the arguments container. // Simple matcher for elements of the arguments container.
class IsKeyEqualTo class IsKeyEqualTo
{ {
public: public:
IsKeyEqualTo(const CStr& value) : m_Value(value) {} IsKeyEqualTo(const CStr& value) : m_Value(value) {}
bool operator()(const std::pair<CStr, CStr>& p) const bool operator()(const std::pair<CStr, CStr>& p) const
{ {
return p.first == m_Value; return p.first == m_Value;
} }
private: private:
const CStr m_Value; const CStr m_Value;
}; };
} // namespace } // namespace
CmdLineArgs::CmdLineArgs(int argc, const char* argv[]) CmdLineArgs::CmdLineArgs(const PS::span<const char* const> argv)
{
if (argc >= 1)
{ {
if (argv.empty())
return;
std::string arg0(argv[0]); std::string arg0(argv[0]);
// avoid OsPath complaining about mixing both types of separators, // avoid OsPath complaining about mixing both types of separators,
// which happens when running in the VC2010 debugger // which happens when running in the VC2010 debugger
std::replace(arg0.begin(), arg0.end(), '/', SYS_DIR_SEP); std::replace(arg0.begin(), arg0.end(), '/', SYS_DIR_SEP);
m_Arg0 = arg0; m_Arg0 = arg0;
}
for (int i = 1; i < argc; ++i) // Implicit conversion from const char* to std::string_view.
for (const std::string_view arg : argv.subspan(1))
StanUnsubmitted
Done
Inline Actions

const std::string_view& ?

Stan: const std::string_view& ?
phositAuthorUnsubmitted
Done
Inline Actions

Normaly for-loops uses & to reference elements in the container but the span dos not hold string_view so it has to be created either way.
I thought with the coment it is explicit enought. Should i use const char* and convert it in the body?

phosit: Normaly for-loops uses `&` to reference elements in the container but the span dos not hold…
vladislavbelovUnsubmitted
Not Done
Inline Actions

I'd suggest arg instead of argN, because N is usually used as a count not as iteration index.

vladislavbelov: I'd suggest `arg` instead of `argN`, because `N` is usually used as a count not as iteration…
{ {
// Only accept arguments that start with '-' // Only accept arguments that start with '-'
if (argv[i][0] != '-') if (arg[0] != '-')
{ {
m_ArgsWithoutName.emplace_back(argv[i]); m_ArgsWithoutName.emplace_back(arg);
vladislavbelovUnsubmitted
Done
Inline Actions

CStr.

vladislavbelov: `CStr`.
continue; continue;
} }
// Allow -arg and --arg // Allow -arg and --arg
char offset = argv[i][1] == '-' ? 2 : 1; const std::string_view afterDashes = arg.substr(arg[1] == '-' ? 2 : 1);
wraitiiUnsubmitted
Not Done
Inline Actions

Isn't argN[1] potentially undefined here? Earlier code had the same flaw TBH.

I also note that you use .front() above, I think for consistency using [0] would make sense but it's debatable.

wraitii: Isn't `argN[1]` potentially undefined here? Earlier code had the same flaw TBH. I also note…
vladislavbelovUnsubmitted
Not Done
Inline Actions

Technically argN[0] might be \0 character for some very rare case when a string is empty, so accessing argN[1] is unsafe.

vladislavbelov: Technically argN[0] might be `\0` character for some very rare case when a string is empty, so…
phositAuthorUnsubmitted
Done
Inline Actions

if argN[0] is \0 this path will not be taken.

phosit: if `argN[0]` is `\0` this path will not be taken.
CStr name, value;
// Check for "-arg=value" // Check for "-arg=value"
const char* eq = strchr(argv[i], '='); const std::string_view::iterator eq =
if (eq) std::find(afterDashes.begin(), afterDashes.end(), '=');
{
name = CStr(argv[i]+offset, eq-argv[i]-offset); const CStr value = (eq != afterDashes.end()) ? CStr{eq + 1, afterDashes.end()} : CStr{};
value = CStr(eq+1);
}
else
name = CStr(argv[i]+offset);
vladislavbelovUnsubmitted
Done
Inline Actions

It might be better to group visually arguments related to the same container.

const std::string_view::iterator eq = std::find(
	afterDashes.begin(), afterDashes.end(), '=');
vladislavbelov: It might be better to group visually arguments related to the same container. ```lang=cpp const…
vladislavbelovUnsubmitted
Not Done
Inline Actions

The distance isn't neccessary here.

const CStr value = (eq != afterDashes.end()) ? CStr(eq + 1, afterDashes.end()) : CStr();
vladislavbelov: The distance isn't neccessary here. ```lang=cpp const CStr value = (eq != afterDashes.end()) ?
phositAuthorUnsubmitted
Done
Inline Actions

To me the distance way is more readable

phosit: To me the `distance` way is more readable
vladislavbelovUnsubmitted
Not Done
Inline Actions

You don't need information about distance, you only need to know is there = or not.

vladislavbelov: You don't need information about distance, you only need to know is there `=` or not.
phositAuthorUnsubmitted
Done
Inline Actions

What happens if eq + 1 == afterDashes.end()? is it a valid CStr? yes it is. but sombody who is reading the code have to thing about.

phosit: What happens if `eq + 1 == afterDashes.end()`? is it a valid CStr? yes it is. but sombody who…
vladislavbelovUnsubmitted
Not Done
Inline Actions

Currently the reader should think why distance = 1 is invalid length.

vladislavbelov: Currently the reader should think why `distance = 1` is invalid length.
m_Args.emplace_back(std::move(name), std::move(value)); m_Args.emplace_back(CStr(afterDashes.begin(), eq), value);
vladislavbelovUnsubmitted
Done
Inline Actions

emplace_back. Also we write an opening parenthesis on the same line.

vladislavbelov: `emplace_back`. Also we write an opening parenthesis on the same line.
vladislavbelovUnsubmitted
Not Done
Inline Actions

value might be moved.

vladislavbelov: `value` might be moved.
phositAuthorUnsubmitted
Done
Inline Actions

I dont thing there is any gain in moving. Performance does not realy mater here

phosit: I dont thing there is any gain in moving. Performance does not realy mater here
vladislavbelovUnsubmitted
Not Done
Inline Actions

Does it make sense to copy if we can move?

vladislavbelov: Does it make sense to copy if we can move?
phositAuthorUnsubmitted
Done
Inline Actions

std::move can be dangerous keeping track of moved-from-objects. Therefore i use it rearly. This case is trivial, i don't care.

phosit: `std::move` can be dangerous keeping track of moved-from-objects. Therefore i use it rearly.
} }
} }
bool CmdLineArgs::Has(const CStr& name) const bool CmdLineArgs::Has(const CStr& name) const
{ {
return std::any_of(m_Args.begin(), m_Args.end(), IsKeyEqualTo(name)); return std::any_of(m_Args.begin(), m_Args.end(), IsKeyEqualTo(name));
StanUnsubmitted
Done
Inline Actions

Missing some includes
<utility> std::piecewise_construct
<tuple> std::tuple
<algorithm> std::min

Stan: Missing some includes <utility> std::piecewise_construct <tuple> std::tuple <algorithm> std…
phositAuthorUnsubmitted
Done
Inline Actions

<utility> is included in the header pair is used there.

phosit: `<utility>` is included in the header `pair` is used there.
} }
StanUnsubmitted
Done
Inline Actions

Apparently this is UB :)

Stan: Apparently this is UB :)
phositAuthorUnsubmitted
Done
Inline Actions

It should not be. It's possible to do it in constexpr which disallows undefined behavior: https://godbolt.org/z/371rc11TW

phosit: It should not be. It's possible to do it in constexpr which disallows undefined behavior: https…
StanUnsubmitted
Done
Inline Actions

What if eq is .end() ?

Stan: What if eq is .end() ?
vladislavbelovUnsubmitted
Done
Inline Actions

Unfortunately you can't trust compilers: https://godbolt.org/z/r3W8Wq8fT.

There is a way to prove it's the UB via standard but I'll show a simpler way.

  1. eq + 1 == std::advance(eq, 1)
  2. https://en.cppreference.com/w/cpp/iterator/advance

The behavior is undefined if the specified sequence of increments or decrements would require that a non-incrementable iterator (such as the past-the-end iterator) is incremented, or that a non-decrementable iterator (such as the front iterator or the singular iterator) is decremented.

vladislavbelov: Unfortunately you can't trust compilers: https://godbolt.org/z/r3W8Wq8fT. There is a way to…
phositAuthorUnsubmitted
Done
Inline Actions

thanks for the citation... always learning something new.
++std::min(eq, --afterDashes.end()) would be a branch-less version but i think now it is more expresive.

What if eq is .end()?

eq < .end() evaluates to false: .end() is taken and the string is empty. I hope that is defined behavior.

phosit: thanks for the citation... always learning something new. `++std::min(eq, --afterDashes.end())`…
CStr CmdLineArgs::Get(const CStr& name) const CStr CmdLineArgs::Get(const CStr& name) const
wraitiiUnsubmitted
Done
Inline Actions

Opinion: a regular if will be more readable here.

wraitii: Opinion: a regular `if` will be more readable here.
{ {
ArgsT::const_iterator it = std::find_if(m_Args.begin(), m_Args.end(), IsKeyEqualTo(name)); ArgsT::const_iterator it = std::find_if(m_Args.begin(), m_Args.end(), IsKeyEqualTo(name));
return it != m_Args.end() ? it->second : ""; return it != m_Args.end() ? it->second : "";
} }
std::vector<CStr> CmdLineArgs::GetMultiple(const CStr& name) const std::vector<CStr> CmdLineArgs::GetMultiple(const CStr& name) const
{ {
std::vector<CStr> values; std::vector<CStr> values;
Show All 19 Lines
Wildfire Games · Phabricator