Skip to content

Remove deprecated Clang-Format style options and setting BreakAfterReturnType to ExceptShortType #5034

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.

Sign up for GitHub

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

Draft
anupsdf wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Remove deprecated Clang-Format style options and setting BreakAfterReturnType to ExceptShortType #5034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
21 changes: 10 additions & 11 deletions .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: None
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterReturnType: AllDefinitions
AlwaysBreakBeforeMultilineStrings: false
AlwaysBreakTemplateDeclarations: false
BinPackArguments: true
BinPackParameters: true
BraceWrapping:
Expand All @@ -38,18 +36,17 @@ BraceWrapping:
SplitEmptyFunction: true
SplitEmptyRecord: true
SplitEmptyNamespace: true
BreakAfterReturnType: ExceptShortType
Copy link
Copy Markdown
Contributor

@drebelsky drebelsky Nov 25, 2025

Choose a reason for hiding this comment

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

Are there not fewer changes with BreakAfterReturnType: AllDefinitions?

Copy link
Copy Markdown
Contributor Author

@anupsdf anupsdf Nov 25, 2025

Choose a reason for hiding this comment

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

I thought ExceptShortType was more readable than AllDefinitions. Though AllDefinitions is closer to what we had before. I will leave it for the team to decide on which one they prefer.

Copy link
Copy Markdown
Contributor

@dmkozh dmkozh Nov 25, 2025

Choose a reason for hiding this comment

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

I would vote for never breaking after the function return type. I don't think it adds any readability, but it definitely does make search for the functions harder. I sometimes search for something like int foo and the line break makes this impossible to do without involving regexes.

Copy link
Copy Markdown
Contributor Author

@anupsdf anupsdf Nov 25, 2025

Choose a reason for hiding this comment

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

There is a None option which sounds like would never break but its marked as deprecated in https://releases.llvm.org/20.1.0/tools/clang/docs/ClangFormatStyleOptions.html

We should do a team poll to vote on the available options: Automatic, ExceptShortType, All, TopLevel, AllDefinitions, TopLevelDefinitions

Copy link
Copy Markdown
Contributor Author

@anupsdf anupsdf Dec 1, 2025

Choose a reason for hiding this comment

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

Here is a difference between ExceptShortType and Automatic,


==========
BreakAfterReturnType: ExceptShortType
==========

void Peer::maybeExecuteInBackground(
    std::string const& jobName, std::function<void(std::shared_ptr<Peer>)> f)
{


==========
BreakAfterReturnType: Automatic
==========

void
Peer::maybeExecuteInBackground(std::string const& jobName,
                               std::function<void(std::shared_ptr<Peer>)> f)
{

BreakBeforeBinaryOperators: None
BreakBeforeBraces: Custom
BreakBeforeInheritanceComma: false
BreakBeforeTernaryOperators: true
BreakConstructorInitializersBeforeComma: true
BreakConstructorInitializers: BeforeComma
BreakAfterJavaFieldAnnotations: false
BreakInheritanceList: AfterComma
BreakStringLiterals: true
BreakTemplateDeclarations: Leave
ColumnLimit: 80
CommentPragmas: '^ IWYU pragma:'
CompactNamespaces: false
ConstructorInitializerAllOnOneLineOrOnePerLine: true
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: true
Expand Down Expand Up @@ -78,14 +75,18 @@ IndentWidth: 4
IndentWrappedFunctionNames: false
JavaScriptQuotes: Leave
JavaScriptWrapImports: true
KeepEmptyLinesAtTheStartOfBlocks: true
KeepEmptyLines:
AtStartOfBlock: false
AtStartOfFile: false
AtEndOfFile: false
MacroBlockBegin: ''
MacroBlockEnd: ''
MaxEmptyLinesToKeep: 1
NamespaceIndentation: None
ObjCBlockIndentWidth: 2
ObjCSpaceAfterProperty: false
ObjCSpaceBeforeProtocolList: true
PackConstructorInitializers: NextLine
Copy link
Copy Markdown
Contributor

@drebelsky drebelsky Nov 25, 2025

Choose a reason for hiding this comment

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

I believe CurrentLine corresponds to what we already have

ConstructorInitializerAllOnOneLineOrOnePerLine (Boolean) clang-format 3.7
This option is deprecated. See CurrentLine of PackConstructorInitializers

PCIS_CurrentLine (in configuration: CurrentLine) Put all constructor initializers on the current line if they fit. Otherwise, put each one on its own line.
PCIS_NextLine (in configuration: NextLine) Same as PCIS_CurrentLine except that if all constructor initializers do not fit on the current line, try to fit them on the next line.

Copy link
Copy Markdown
Contributor Author

@anupsdf anupsdf Nov 25, 2025

Choose a reason for hiding this comment

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

NextLine seemed a bit more readable but we can check with team's preference.

Copy link
Copy Markdown
Contributor Author

@anupsdf anupsdf Dec 1, 2025

Choose a reason for hiding this comment

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

Here is a difference between NextLine and CurrentLine,

==========
PackConstructorInitializers: NextLine
==========

CatchupConfiguration::CatchupConfiguration(LedgerNumHashPair ledgerHashPair,
                                           uint32_t count, Mode mode)
    : mCount{count}, mLedgerHashPair{ledgerHashPair}, mMode{mode}
{
}

==========
PackConstructorInitializers: CurrentLine
==========
CatchupConfiguration::CatchupConfiguration(LedgerNumHashPair ledgerHashPair,
                                           uint32_t count, Mode mode)
    : mCount{count}
    , mLedgerHashPair{ledgerHashPair}
    , mMode{mode}
{
}

PenaltyBreakAssignment: 2
PenaltyBreakBeforeFirstCallParameter: 19
PenaltyBreakComment: 300
Expand All @@ -101,14 +102,12 @@ SpaceAfterCStyleCast: false
SpaceAfterTemplateKeyword: true
SpaceBeforeAssignmentOperators: true
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles: false
SpacesInContainerLiterals: true
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInParens: Never
SpacesInSquareBrackets: false
Standard: Cpp11
Standard: c++17
TabWidth: 8
UseTab: Never
UseCRLF: false
Expand Down
41 changes: 15 additions & 26 deletions src/bucket/BucketApplicator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,29 +57,25 @@ BucketApplicator::BucketApplicator(Application& app,
}
}

BucketApplicator::
operator bool() const
BucketApplicator::operator bool() const
{
// There is more work to do (i.e. (bool) *this == true) iff:
// 1. The underlying bucket iterator is not EOF and
// 2. We have offers still remaining.
return static_cast<bool>(mBucketIter) && mOffersRemaining;
}

size_t
BucketApplicator::pos()
size_t BucketApplicator::pos()
{
return mBucketIter.pos();
}

size_t
BucketApplicator::size() const
size_t BucketApplicator::size() const
{
return mBucketIter.size();
}

static bool
shouldApplyEntry(BucketEntry const& e)
static bool shouldApplyEntry(BucketEntry const& e)
{
if (e.type() == LIVEENTRY || e.type() == INITENTRY)
{
Expand All @@ -94,8 +90,7 @@ shouldApplyEntry(BucketEntry const& e)
return LiveBucketIndex::typeNotSupported(e.deadEntry().type());
}

size_t
BucketApplicator::advance(BucketApplicator::Counters& counters)
size_t BucketApplicator::advance(BucketApplicator::Counters& counters)
{
size_t count = 0;

Expand Down Expand Up @@ -217,8 +212,7 @@ BucketApplicator::Counters::Counters(VirtualClock::time_point now)
reset(now);
}

void
BucketApplicator::Counters::reset(VirtualClock::time_point now)
void BucketApplicator::Counters::reset(VirtualClock::time_point now)
{
mStarted = now;
for (auto let : xdr::xdr_traits<LedgerEntryType>::enum_values())
Expand All @@ -228,8 +222,7 @@ BucketApplicator::Counters::reset(VirtualClock::time_point now)
}
}

void
BucketApplicator::Counters::getRates(
void BucketApplicator::Counters::getRates(
VirtualClock::time_point now,
std::map<LedgerEntryType, CounterEntry>& sec_counters, uint64_t& T_sec,
uint64_t& total)
Expand All @@ -248,8 +241,7 @@ BucketApplicator::Counters::getRates(
T_sec = (total * 1000000) / usecs;
}

std::string
BucketApplicator::Counters::logStr(
std::string BucketApplicator::Counters::logStr(
uint64_t total, uint64_t level, std::string const& bucketName,
std::map<LedgerEntryType, CounterEntry> const& counters)
{
Expand All @@ -266,10 +258,9 @@ BucketApplicator::Counters::logStr(
return str;
}

void
BucketApplicator::Counters::logInfo(std::string const& bucketName,
uint32_t level,
VirtualClock::time_point now)
void BucketApplicator::Counters::logInfo(std::string const& bucketName,
uint32_t level,
VirtualClock::time_point now)
{
uint64_t T_sec, total;
std::map<LedgerEntryType, CounterEntry> sec_counters;
Expand All @@ -280,10 +271,9 @@ BucketApplicator::Counters::logInfo(std::string const& bucketName,
logStr(total, level, bucketName, mCounters));
}

void
BucketApplicator::Counters::logDebug(std::string const& bucketName,
uint32_t level,
VirtualClock::time_point now)
void BucketApplicator::Counters::logDebug(std::string const& bucketName,
uint32_t level,
VirtualClock::time_point now)
{
uint64_t T_sec, total;
std::map<LedgerEntryType, CounterEntry> sec_counters;
Expand All @@ -292,8 +282,7 @@ BucketApplicator::Counters::logDebug(std::string const& bucketName,
logStr(total, level, bucketName, sec_counters), T_sec);
}

void
BucketApplicator::Counters::mark(BucketEntry const& e)
void BucketApplicator::Counters::mark(BucketEntry const& e)
{
if (e.type() == LIVEENTRY || e.type() == INITENTRY)
{
Expand Down
37 changes: 13 additions & 24 deletions src/bucket/BucketBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ namespace stellar
{

template <class BucketT, class IndexT>
IndexT const&
BucketBase<BucketT, IndexT>::getIndex() const
IndexT const& BucketBase<BucketT, IndexT>::getIndex() const
{
ZoneScoped;
releaseAssertOrThrow(!mFilename.empty());
Expand All @@ -42,15 +41,14 @@ BucketBase<BucketT, IndexT>::getIndex() const
}

template <class BucketT, class IndexT>
bool
BucketBase<BucketT, IndexT>::isIndexed() const
bool BucketBase<BucketT, IndexT>::isIndexed() const
{
return static_cast<bool>(mIndex);
}

template <class BucketT, class IndexT>
void
BucketBase<BucketT, IndexT>::setIndex(std::unique_ptr<IndexT const>&& index)
void BucketBase<BucketT, IndexT>::setIndex(
std::unique_ptr<IndexT const>&& index)
{
releaseAssertOrThrow(!mIndex);
mIndex = std::move(index);
Expand All @@ -76,29 +74,25 @@ template <class BucketT, class IndexT> BucketBase<BucketT, IndexT>::BucketBase()
}

template <class BucketT, class IndexT>
Hash const&
BucketBase<BucketT, IndexT>::getHash() const
Hash const& BucketBase<BucketT, IndexT>::getHash() const
{
return mHash;
}

template <class BucketT, class IndexT>
std::filesystem::path const&
BucketBase<BucketT, IndexT>::getFilename() const
std::filesystem::path const& BucketBase<BucketT, IndexT>::getFilename() const
{
return mFilename;
}

template <class BucketT, class IndexT>
size_t
BucketBase<BucketT, IndexT>::getSize() const
size_t BucketBase<BucketT, IndexT>::getSize() const
{
return mSize;
}

template <class BucketT, class IndexT>
bool
BucketBase<BucketT, IndexT>::isEmpty() const
bool BucketBase<BucketT, IndexT>::isEmpty() const
{
if (mFilename.empty() || isZero(mHash))
{
Expand All @@ -110,8 +104,7 @@ BucketBase<BucketT, IndexT>::isEmpty() const
}

template <class BucketT, class IndexT>
void
BucketBase<BucketT, IndexT>::freeIndex()
void BucketBase<BucketT, IndexT>::freeIndex()
{
mIndex.reset(nullptr);
}
Expand Down Expand Up @@ -182,8 +175,7 @@ BucketBase<BucketT, IndexT>::randomBucketIndexName(std::string const& tmpDir)
// bucket enters the youngest level. At least one new bucket is in every merge's
// shadows from then on in, so they all upgrade (and preserve lifecycle events).
template <class BucketT, class IndexT>
static void
calculateMergeProtocolVersion(
static void calculateMergeProtocolVersion(
MergeCounters& mc, uint32_t maxProtocolVersion,
BucketInputIterator<BucketT> const& oi,
BucketInputIterator<BucketT> const& ni,
Expand Down Expand Up @@ -238,8 +230,7 @@ calculateMergeProtocolVersion(
// not scrutinizing the entry type further.
template <class BucketT, class IndexT, typename InputSource,
typename... ShadowParams>
static bool
mergeCasesWithDefaultAcceptance(
static bool mergeCasesWithDefaultAcceptance(
BucketEntryIdCmp<BucketT> const& cmp, MergeCounters& mc,
InputSource& inputSource,
std::function<void(typename BucketT::EntryT const&)> putFunc,
Expand Down Expand Up @@ -285,8 +276,7 @@ mergeCasesWithDefaultAcceptance(

template <class BucketT, class IndexT>
template <typename InputSource, typename PutFuncT, typename... ShadowParams>
void
BucketBase<BucketT, IndexT>::mergeInternal(
void BucketBase<BucketT, IndexT>::mergeInternal(
BucketManager& bucketManager, InputSource& inputSource, PutFuncT putFunc,
uint32_t protocolVersion, MergeCounters& mc, ShadowParams&&... shadowParams)
{
Expand Down Expand Up @@ -337,8 +327,7 @@ BucketBase<BucketT, IndexT>::mergeInternal(
}

template <class BucketT, class IndexT>
std::shared_ptr<BucketT>
BucketBase<BucketT, IndexT>::merge(
std::shared_ptr<BucketT> BucketBase<BucketT, IndexT>::merge(
BucketManager& bucketManager, uint32_t maxProtocolVersion,
std::shared_ptr<BucketT> const& oldBucket,
std::shared_ptr<BucketT> const& newBucket,
Expand Down
3 changes: 1 addition & 2 deletions src/bucket/BucketBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,7 @@ class BucketBase : public NonMovableOrCopyable
static std::string randomBucketIndexName(std::string const& tmpDir);

#ifdef BUILD_TESTS
IndexT const&
getIndexForTesting() const
IndexT const& getIndexForTesting() const
{
return getIndex();
}
Expand Down
3 changes: 1 addition & 2 deletions src/bucket/BucketIndexUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@
namespace stellar
{

std::streamoff
getPageSizeFromConfig(Config const& cfg)
std::streamoff getPageSizeFromConfig(Config const& cfg)
{
if (cfg.BUCKETLIST_DB_INDEX_PAGE_SIZE_EXPONENT == 0)
{
Expand Down
9 changes: 3 additions & 6 deletions src/bucket/BucketIndexUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,17 @@ class IndexReturnT
{
}

IndexReturnState
getState() const
IndexReturnState getState() const
{
return mState;
}
IndexPtrT
cacheHit() const
IndexPtrT cacheHit() const
{
releaseAssertOrThrow(mState == IndexReturnState::CACHE_HIT);
releaseAssertOrThrow(std::holds_alternative<IndexPtrT>(mPayload));
return std::get<IndexPtrT>(mPayload);
}
std::streamoff
fileOffset() const
std::streamoff fileOffset() const
{
releaseAssertOrThrow(mState == IndexReturnState::FILE_OFFSET);
releaseAssertOrThrow(std::holds_alternative<std::streamoff>(mPayload));
Expand Down
Loading