code style questions

[scarf005]

auto is discouraged in https://github.com/cataclysmbnteam/Cataclysm-BN/blob/ef6860115c9a0a2521b360e2a722aec2593d4ccf/doc/CODE_STYLE.md#code-guidelines.

Auto Almost Nothing. Auto has two uses, others should be avoided.
Aliasing for extremely long iterator or functional declarations.
Generic code support (but decltype is better).

it'd be great to add more reasoning on discouraging auto, with cases where using classic C++ style would be beneficial,
since auto has some advantages in certain contexts:

function returns

// Classic C++ function declaration
std::vector<int> foo_func1() 
{ 
  // ...
}

// Modern C++ style
auto foo_func2() -> std::vector<int>
{ 
  // ...
}

advantages listed in https://stackoverflow.com/a/11215414/13503626:

• names align left, improving readability
• consistent style with lambda, where trailing return is mandatory
• its usage is mandatory when using decltype
• member typedefs are in scope

struct my_awesome_type {
    typedef std::vector<int> integer_sequence;
    integer_sequence get_integers() const;
}; 

// Classic C++ style
my_awesome_type::integer_sequence my_awesome_type::get_integers() const {
    // ...
}

// Modern C++ style
auto my_awesome_type::get_integers() const -> integer_sequence {
    // ...
}

variable declaration

variable names aligned left and types on right gives better readability.

// Classic C++ declaration order     // Modern C++ style

const char* s = "Hello";             auto s = "Hello";
widget w = get_widget();             auto w = get_widget();

employee e{ empid };                 auto e = employee{ empid };
widget w{ 12, 34 };                  auto w = widget{ 12, 34 };

this gets more prominent as types get verbose:

// Classic C++ declaration order
std::vector<std::vector<int>> nums_1 = {{1, 2}, {3, 4}};
std::array<std::pair<std::string, std::string>, 2> pairs_1 = {{{"a", "1"}, {"b", "2"}}};

// Modern C++ style
auto nums_2 = std::vector<std::vector<int>>{{1, 2}, {3, 4}};
auto pairs_2 = std::array<std::pair<std::string, std::string>, 2>{{{"a", "1"}, {"b", "2"}}};

in above case, the name nums and pairs are easier to find. (regardless of using typedefs)
there is a caveat that uncopyable and unmovable class cannot be initialized this way, tho.

Additional Context

against auto:

https://josephmansfield.uk/articles/dont-use-auto-unless-you-mean-it.html

in favor of auto:

https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/
https://stackoverflow.com/questions/11215227/should-the-trailing-return-type-syntax-style-become-the-default-for-new-c11-pr

[olanti-p]

While I agree with most of these points in favor of auto in general (Rust uses similar style, where names go before types, and it's quite convenient), I'm not sure whether they'd be viable for BN.

function returns

That's the first time I'm seeing such "modern" style tbh. Everyone is familiar with the classic style, almost every codebase out there uses it, all our dependencies use it, and I highly doubt we have any usages of the "modern" style either.

Using auto here would just go against the unspoken established style, and further confuse many contributors who don't have a strong grasp on modern C++ features.

Alternative solution here is just to separate return type and function name by a newline

// Classic C++ with newline
std::vector<int>
foo_func1() 
{ 
  // ...
}

// Classic C++ with newline
my_awesome_type::integer_sequence
my_awesome_type::get_integers() const {
    // ...
}

variable declaration

Similar issue here: auto may be hard to grasp for contributors who don't have much experience.

Long types often mean the relevant part lacks proper encapsulation, or the programmer is just too lazy to add a struct with a couple named fields and instead crams everything into a pair or a tuple, which makes it hard to understand when a field is accessed. Also, typedef and using exist.

In the end, modern text editors and IDEs often just color code the identifier, which makes it stand out to the naked eye (screenshot from VSCode).

Common use of auto in this case also incentivizes using it everywhere, even in trivial cases like auto x = 3;

[scarf005]

Fair point. However I think new contributors would be okay with using trailing auto return for everything, since we use a fair amount of lambdas.

before:

Cataclysm-BN/src/character.cpp

Lines 10230 to 10239 in b0d5f5a

	std::vector<Creature *> Character::get_hostile_creatures( int range ) const
	{
	return g->get_creatures_if( [this, range]( const Creature & critter ) -> bool {
	// Fixes circular distance range for ranged attacks
	float dist_to_creature = std::round( rl_dist_exact( pos(), critter.pos() ) );
	return this != &critter && pos() != critter.pos() && // TODO: get rid of fake npcs (pos() check)
	dist_to_creature <= range && critter.attitude_to( *this ) == A_HOSTILE
	&& sees( critter );
	} );
	}

after:

auto Character::get_hostile_creatures( int range ) const -> std::vector<Creature *>
{
    return g->get_creatures_if( [this, range]( const Creature & critter ) -> bool {
        // Fixes circular distance range for ranged attacks
        const auto dist_to_creature = std::round( rl_dist_exact( pos(), critter.pos() ) );
        return this != &critter && pos() != critter.pos() && // TODO: get rid of fake npcs (pos() check)
        dist_to_creature <= range && critter.attitude_to( *this ) == A_HOSTILE
        && sees( critter );
    } );
}