Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chestdialogfix #6

Open
wants to merge 121 commits into
base: master
Choose a base branch
from
Open

Chestdialogfix #6

wants to merge 121 commits into from

Conversation

Cheerfulbull
Copy link
Owner

:(

ihhub and others added 30 commits June 17, 2024 10:31
Districh-ru and others added 22 commits July 27, 2024 17:43
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (1/4)

@@ -241,7 +240,7 @@ bool World::LoadMapMP2( const std::string & filename, const bool isOriginalMp2Fi
for ( int32_t i = 0; i < worldSize; ++i ) {
Maps::Tiles & tile = vec_tiles[i];

MP2::mp2tile_t mp2tile;
MP2::MP2TileInfo mp2tile;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-member-init ⚠️
uninitialized record type: mp2tile

Suggested change
MP2::MP2TileInfo mp2tile;
MP2::MP2TileInfo mp2tile{};

return false;
}

for ( const int32_t nearbyIdx : Battle::Board::GetAroundIndexes( pos ) ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-use-anyofallof ⚠️
replace loop by std::any_of()

current.canAttackImmediately = Battle::Board::CanAttackTargetFromPosition( attacker, defender, posHeadIdx );

// Pick target if either position has improved or unit is higher value at the same position value
if ( IsOutcomeImproved( current, bestOutcome ) ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-suspicious-call-argument ⚠️
1st argument current (passed to newOutcome) looks like it might be swapped with the 2nd, bestOutcome (passed to previous)


actions.emplace_back( Battle::Command::SPELLCAST, bestSpell.spellID, bestSpell.cell );

DEBUG_LOG( DBG_BATTLE, DBG_INFO,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-lambda-function-name ⚠️
inside a lambda, __FUNCTION__ expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly

continue;
}

const int32_t archerMeleeDmg = [&currentUnit, enemy]() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-narrowing-conversions ⚠️
narrowing conversion from uint32_t (aka unsigned int) to signed type int32_t (aka int) is implementation-defined

return ( currentUnit.CalculateMinDamage( *enemy ) + currentUnit.CalculateMaxDamage( *enemy ) ) / 2;
}();

const int32_t retaliatoryDmg = enemy->EstimateRetaliatoryDamage( archerMeleeDmg );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-narrowing-conversions ⚠️
narrowing conversion from uint32_t (aka unsigned int) to signed type int32_t (aka int) is implementation-defined

target.cell = targetIdx;
target.unit = enemy;

DEBUG_LOG( DBG_BATTLE, DBG_TRACE, "- Set shooting attack priority on " << enemy->GetName() << ", value: " << priority )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-lambda-function-name ⚠️
inside a lambda, __FUNCTION__ expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly

@@ -22,7 +22,7 @@

#define MAJOR_VERSION 1
#define MINOR_VERSION 1
#define INTERMEDIATE_VERSION 0
#define INTERMEDIATE_VERSION 1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-macro-to-enum ⚠️
macro INTERMEDIATE_VERSION defines an integral constant; prefer an enum instead

@@ -22,7 +22,7 @@

#define MAJOR_VERSION 1
#define MINOR_VERSION 1
#define INTERMEDIATE_VERSION 0
#define INTERMEDIATE_VERSION 1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-macro-usage ⚠️
macro INTERMEDIATE_VERSION used to declare a constant; consider using a constexpr constant

@@ -319,7 +315,7 @@ bool Maps::FileInfo::readMP2Map( std::string filePath, const bool isForEditor )
// Each tile needs 16 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 16 + 32 + 32 = 160 bits or 20 bytes.
fs.seek( MP2::MP2_MAP_INFO_SIZE + ( lossConditionParams[0] + lossConditionParams[1] * width ) * 20 );

MP2::mp2tile_t mp2tile;
MP2::MP2TileInfo mp2tile;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-member-init ⚠️
uninitialized record type: mp2tile

Suggested change
MP2::MP2TileInfo mp2tile;
MP2::MP2TileInfo mp2tile{};

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (2/4)

break;
case DWELLING_MONSTER6:
offset.x = 314;
offset.y = 301;
icnIndex = DWELLING_UPGRADE7 & building ? 30 : ( DWELLING_UPGRADE6 & building ? 29 : 24 );
icnIndex = DWELLING_UPGRADE7 & _constructedBuildings ? 30 : ( DWELLING_UPGRADE6 & _constructedBuildings ? 29 : 24 );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-avoid-nested-conditional-operator ⚠️
conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators

{
uint32_t id2;
uint8_t race;
cost_t cost;
};

const buildstats_t _builds[] = {
const BuildingStats buildingStats[] = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-avoid-c-arrays ⚠️
do not declare C-style arrays, use std::array<> instead

}
}
else if ( MP2::isInGameActionObject( objectType ) ) {
_mapActionObjects.emplace( iter, IndexObject{ mapIndex, objectType } );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ modernize-use-emplace ⚠️
unnecessary temporary object created while calling emplace

Suggested change
_mapActionObjects.emplace( iter, IndexObject{ mapIndex, objectType } );
_mapActionObjects.emplace( iter, mapIndex, objectType );


// XMI files play MIDI at a fixed clock rate of 120 Hz
if ( !tracks.empty() && tracks.front().events.trackTempo > 0 ) {
ppqn = ( tracks.front().events.trackTempo * 3 / 25000 );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-narrowing-conversions ⚠️
narrowing conversion from uint32_t (aka unsigned int) to signed type int is implementation-defined

return {};
}

StreamBuf sb( 16 * 4096 );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-implicit-widening-of-multiplication-result ⚠️
performing an implicit widening conversion to type size_t (aka unsigned long) of a multiplication performed in type int

for ( const BuildOrder & order : buildOrder ) {
const BuildingStatus status = castle->CheckBuyBuilding( order.building );
if ( status == BuildingStatus::LACK_RESOURCES ) {
Funds missing = PaymentConditions::BuyBuilding( race, order.building ) - kindgomFunds;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ misc-const-correctness ⚠️
variable missing of type Funds can be declared const

Suggested change
Funds missing = PaymentConditions::BuyBuilding( race, order.building ) - kindgomFunds;
Funds const missing = PaymentConditions::BuyBuilding( race, order.building ) - kindgomFunds;

budgetEntry.recurringCost = true;
}
if ( castle->isBuild( DWELLING_MONSTER6 ) ) {
Funds bestUnitCost = Monster( race, DWELLING_MONSTER6 ).GetUpgrade().GetCost();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ misc-const-correctness ⚠️
variable bestUnitCost of type Funds can be declared const

Suggested change
Funds bestUnitCost = Monster( race, DWELLING_MONSTER6 ).GetUpgrade().GetCost();
Funds const bestUnitCost = Monster( race, DWELLING_MONSTER6 ).GetUpgrade().GetCost();

if ( toRes == Resource::GOLD ) {
fromResBalanceAmount -= 1;
toResBalanceAmount += tradeCost;
const int32_t tradeCost = fheroes2::getTradeCost( marketplaceCount, fromRes, toRes );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-narrowing-conversions ⚠️
narrowing conversion from uint32_t (aka unsigned int) to signed type int32_t (aka int) is implementation-defined

}

if ( resToExchange == Resource::UNKNOWN ) {
return {};
const int32_t tradeCost = fheroes2::getTradeCost( marketplaceCount, res, missingRes );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-narrowing-conversions ⚠️
narrowing conversion from uint32_t (aka unsigned int) to signed type int32_t (aka int) is implementation-defined

int startAutoBattle
= fheroes2::showMessage( fheroes2::Text( "", {} ), fheroes2::Text( _( "Are you sure you want to enable auto combat?" ), fheroes2::FontType::normalWhite() ),
Dialog::YES | Dialog::NO );
int startAutoBattle = fheroes2::showStandardTextMessage( {}, _( "Are you sure you want to enable auto combat?" ), Dialog::YES | Dialog::NO );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ misc-const-correctness ⚠️
variable startAutoBattle of type int can be declared const

Suggested change
int startAutoBattle = fheroes2::showStandardTextMessage( {}, _( "Are you sure you want to enable auto combat?" ), Dialog::YES | Dialog::NO );
int const startAutoBattle = fheroes2::showStandardTextMessage( {}, _( "Are you sure you want to enable auto combat?" ), Dialog::YES | Dialog::NO );

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (3/4)

uint32_t cur = resource.Get( rs );
uint32_t sel = cur;
uint32_t max = mul > 1 ? ( funds.Get( rs ) + resource.Get( rs ) ) / mul : funds.Get( rs ) + resource.Get( rs );
int32_t cur = resource.Get( rs );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ misc-const-correctness ⚠️
variable cur of type int32_t (aka int) can be declared const

Suggested change
int32_t cur = resource.Get( rs );
int32_t const cur = resource.Get( rs );

case DWELLING_MONSTER6:
return building & DWELLING_UPGRADE7 ? DWELLING_UPGRADE7 : ( building & DWELLING_UPGRADE6 ? DWELLING_UPGRADE6 : buildId );
return _constructedBuildings & DWELLING_UPGRADE7 ? DWELLING_UPGRADE7 : ( _constructedBuildings & DWELLING_UPGRADE6 ? DWELLING_UPGRADE6 : buildId );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-avoid-nested-conditional-operator ⚠️
conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators

while ( currentEntry < regionsToCheck.size() ) {
const MapRegion & region = world.getRegion( regionsToCheck[currentEntry].first );

for ( uint32_t secondaryID : region._neighbours ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ misc-const-correctness ⚠️
variable secondaryID of type uint32_t (aka unsigned int) can be declared const

Suggested change
for ( uint32_t secondaryID : region._neighbours ) {
for ( uint32_t const secondaryID : region._neighbours ) {

Comment on lines +645 to +649
if ( hero && hero->GetArmy().GetStrength() <= enemyStrength * ARMY_ADVANTAGE_SMALL ) {
return true;
}

return false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-simplify-boolean-expr ⚠️
redundant boolean literal in conditional return statement

Suggested change
if ( hero && hero->GetArmy().GetStrength() <= enemyStrength * ARMY_ADVANTAGE_SMALL ) {
return true;
}
return false;
return hero && hero->GetArmy().GetStrength() <= enemyStrength * ARMY_ADVANTAGE_SMALL;

@@ -50,7 +50,7 @@ struct spellstats_t
// The original resources don't have most of sprites for Mass Spells
// so we made some tricks in AGG source file. All modified sprite IDs start from 60

spellstats_t spells[] = {
const SpellStats spells[Spell::SPELL_COUNT] = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-avoid-c-arrays ⚠️
do not declare C-style arrays, use std::array<> instead

const CastleDefenseElement target = Catapult::GetTarget( stateOfCatapultTargets, _randomGenerator );
const uint32_t damage = std::min( _catapult->GetDamage( _randomGenerator ), stateOfCatapultTargets[target] );
const bool hit = _catapult->IsNextShotHit( _randomGenerator );
cmd << shots;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-narrowing-conversions ⚠️
narrowing conversion from uint32_t (aka unsigned int) to signed type int is implementation-defined

void showPopup( const int buttons ) const override;

private:
const Monster _monster;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-avoid-const-or-ref-data-members ⚠️
member _monster of type const Monster is const qualified

const double spellPointValue = retreating ? outcome.value : outcome.value / sqrt( spell.spellPoints( _commander ) / 3.0 );
const bool ignoreThreshold = retreating || spell.isResurrect();

DEBUG_LOG( DBG_BATTLE, DBG_TRACE, spell.GetName() << " value is " << spellPointValue << ", best target is " << outcome.cell )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-lambda-function-name ⚠️
inside a lambda, __FUNCTION__ expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly

// Slow is useless against archers or troops defending castle
return 0.01;
}
const int currentSpeed = target.GetSpeed( false, true );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-narrowing-conversions ⚠️
narrowing conversion from uint32_t (aka unsigned int) to signed type int is implementation-defined


double AI::BattlePlanner::getSpellHasteRatio( const Battle::Unit & target ) const
{
const int currentSpeed = target.GetSpeed( false, true );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-narrowing-conversions ⚠️
narrowing conversion from uint32_t (aka unsigned int) to signed type int is implementation-defined

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (4/4)

return 0.0;
}

if ( target.Modes( Battle::SP_BLESS ) && ( spellID == Spell::CURSE || spellID == Spell::MASSCURSE ) ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ bugprone-branch-clone ⚠️
repeated branch body in conditional chain

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.