Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you haven't received our response, please do the following:
check your Spam/Junk folder and click the
"Not Spam" button for our message.
This way, you won't miss messages from our team in the future.
We have checked YTsaurus with the PVS-Studio analyzer — let's see the results of this check and the errors found. It's been over half a year since YTsaurus, a powerful BigData system, became open source. It has been developed to assist companies in expanding their IT infrastructure and tackling business initiatives. These days, YTsaurus is a frequent topic for discussion. According to GitHub statistics, the project is still growing in popularity. That is why we are here to inspect it.
In fact, YTsaurus is frequently and extensively discussed at various conferences which our team attends. For example, we recently attended the HighLoad++ and TeamLead Conf conferences, and YTsaurus-related topics were everywhere.
Considering the popularity of the project and my interests in related fields, I started exploring it. YTsaurus, being an open-source solution available to every user, happens to have a complete documentation, and I turned to it. The home page has both a brief description of the project and a full-fledged Overview section. It explains the YTsaurus purpose, describes key features of the platform and the structure of the YTsaurus system.
So, if you're curious, read the section in full — it's going to be even more interesting. Well, I will not say anything new about the project, except for the results of the code check, of course. By the way, the project source code is available here.
Even before I finished checking the project, GitHub was stuffed with new commits. This is a clear indication that the project is under active development.
The commit I cloned the project on was 55cd85f.
This article is not intended to devalue the labor of the YTsaurus developers. The aim is to popularize static code analyzers, which can be of great assistance even for high-quality and successful projects.
We didn't, actually. "Why have such a catchy headline?", you may ask.
I would like to express my special thanks to the developers of the project! What detailed instructions YTsaurus has! And how simple YTsaurus is to deploy. I do think that even children would be able to deploy YTsaurus if they wanted to.
Well, let's go straight to what we are all looking forward to — code analysis.
...but a bunch of different warnings.
If the project is large enough, the analyzer may find many suspicious code fragments. This may spoil the first impression of the tool. A scared user will immediately uninstall the analyzer from their machine.
How can we avoid it?
When checking projects from time to time, we usually don't go through all of these steps. However, we do perform some sort of configuration before starting the analysis: we exclude third-party libraries, enable only General Analysis and micro-optimizations diagnostic groups. Sometimes even the finely tuned analyzer may issue a lot of warnings, but it didn't take me long to analyze the errors found in YTsaurus.
While reviewing and examining the analysis results, I found many curious and not-so-curious code snippets that I want to share with you.
Fragment N1
TAsyncSignalsHandler *SIGNALS_HANDLER = nullptr;
void SetAsyncSignalHandler(int signum, TAutoPtr<TEventHandler> handler)
{
static TAdaptiveLock lock;
if (Y_UNLIKELY(SIGNALS_HANDLER == nullptr)) // N1
{
TGuard dnd(lock);
if (SIGNALS_HANDLER == nullptr) // N2
{
// NEVERS GETS DESTROYED
SIGNALS_HANDLER = new TAsyncSignalsHandler(); // N3
}
}
SIGNALS_HANDLER->Install(signum, handler); // N4
}The analyzer warning: V547 Expression 'SIGNALS_HANDLER == nullptr' is always true. async_signals_handler.cpp:200
Although the analyzer issued the V547 warning (it didn't get the custom locking mechanism quite right), I detected a curious example of the Double-checked locking pattern.
What can go wrong? Firstly, we have a small chance that the compiler may cache the value of the SIGNALS_HANDLER variable in a register and not read the new value again. At least if unusual locking is used. The best solution is to declare the variable volatile or use std::atomic.
That's not all. Let's figure out what's wrong in this case. Let us assume that the A and B threads operate with this code.
The A thread is the first to reach the N1 point. The pointer is null, so the control flow reaches the first if and catches the lock object.
The A thread reaches the N2 point. The pointer is also null, so the control flow reaches the second if.
Then, a little magic can happen at the N3 point. The pointer initialization can be roughly represented as follows:
auto tmp = (TAsyncSignalsHandler *) malloc(sizeof(TAsyncSignalsHandler));
new (tmp) TAsyncSignalsHandler();
SIGNALS_HANDLER = tmp;A clever processor can reorder the instructions, and SIGNALS_HANDLER will be initialized before placement new is called.
And then the B thread joins the battle — exactly when the A thread hasn't yet called placement new. The control flow enters the N1 point, detects the initialized pointer, and moves to the N4 point.
In the B thread, the Install member function is called on the object that has not yet started its lifetime. Undefined behavior...
How to fix this pattern? We need to make reading and writing to SIGNALS_HANDLER be executed as atomic operations:
std::atomic<TAsyncSignalsHandler *> SIGNALS_HANDLER { nullptr };
void SetAsyncSignalHandler(int signum, TAutoPtr<TEventHandler> handler)
{
static TAdaptiveLock lock;
auto tmp = SIGNALS_HANDLER.load(std::memory_order_acquire); // <=
if (Y_UNLIKELY(tmp == nullptr))
{
TGuard dnd(lock);
tmp = SIGNALS_HANDLER.load(std::memory_order_relaxed); // <=
if (tmp == nullptr)
{
// NEVERS GETS DESTROYED
tmp = new TAsyncSignalsHandler();
SIGNALS_HANDLER.store(tmp, std::memory_order_release); // <=
}
}
tmp->Install(signum, handler);
}Fragment N2
TSimpleTM& TSimpleTM::Add(EField f, i32 amount)
{
....
case F_YEAR:
{
i32 y = amount + (i32)Year;
y = ::Min<i32>(Max<i32>(y, 0), 255 /*max year*/);
// YDay may correspond to different MDay if
// it's March or greater and the years have different leap status
if (Mon > 1)
{
YDay += (i32)LeapYearAD(RealYear()) - (i32)LeapYearAD(RealYear()); // <=
}
Year = y;
IsLeap = LeapYearAD(RealYear());
return RegenerateFields();
}
....
}The analyzer warning: V501 There are identical sub-expressions '(i32) LeapYearAD(RealYear())' to the left and to the right of the '-' operator. datetime.cpp:154:1
It seems that left and right expressions should not be identical. But they are, and it results in YDay += 0 always occurring.
Sure, we don't know the whole context and therefore can't say with certainty what should be going on in that line and what code should be there. But in this case, the analyzer is right.
I guess the developer got sidetracked while coding.
Fragment N3
bool operator == (const TCompositePendingJobCount& lhs,
const TCompositePendingJobCount& rhs)
{
if (lhs.DefaultCount != rhs.DefaultCount)
{
return false;
}
if (lhs.CountByPoolTree.size() != lhs.CountByPoolTree.size()) // <=
{
return false;
}
for (const auto& [tree, lhsCount] : lhs.CountByPoolTree)
{
....
}
return true;
}The analyzer warning: V501 There are identical sub-expressions 'lhs.CountByPoolTree.size()' to the left and to the right of the '!=' operator. structs.cpp:191
This is another error caused by distraction. The size of the same container is being compared, which is why the expression is always false.
The correct code looks like this:
if (lhs.CountByPoolTree.size() != rhs.CountByPoolTree.size()){....}Actually, comparisons often run into errors. Not all of these errors detected by the analyzer are described in this article. Errors in comparisons are thoroughly described in my colleague's article: "The evil within the comparison functions". I strongly recommend you read it.
Fragment N4.
template <class TOptions>
class TSimpleOperationCommandBase
: public virtual TTypedCommandBase<TOptions>
{
....
public:
TSimpleOperationCommandBase()
{
....
if (!OperationId.IsEmpty() && OperationAlias.operator bool() ||
OperationId.IsEmpty() && !OperationAlias.operator bool() )
{
....
}
....
}
};The analyzer warning: V728 – Message: An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. scheduler_commands.h:36:1
It may be easier for the programmer who wrote the code to perceive the code above. Basically, this expression implies the use of the mutually exclusive OR operator for two Boolean operands. In C and C++, in addition to the bitwise mutually exclusive OR operator, there is also an implicit logical comparison. This comparison is implemented via the "!=" operator, and we can use it to rewrite the expression in the following way:
if (OperationId.IsEmpty() != OperationAlias.operator bool())The mutually exclusive OR operator returns true only when operands are unequal. And this is exactly the case where we need the inequality (!=) operator. As a result, the code becomes more concise.
Here is... another example.
Fragment N5.
int TComparator::CompareKeyBounds(const TKeyBound& lhs,
const TKeyBound& rhs,
int lowerVsUpper) const
{
....
// Prefixes coincide. Check if key bounds are indeed at the same point.
{
auto lhsInclusivenessAsUpper = (lhs.IsUpper && lhs.IsInclusive) ||
(!lhs.IsUpper && !lhs.IsInclusive);
auto rhsInclusivenessAsUpper = (rhs.IsUpper && rhs.IsInclusive) ||
(!rhs.IsUpper && !rhs.IsInclusive);
if (lhsInclusivenessAsUpper != rhsInclusivenessAsUpper)
{
return lhsInclusivenessAsUpper - rhsInclusivenessAsUpper;
}
}
....
}The analyzer warning: V728 – Message: An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression. comparator.cpp:194:1
This code can be shortened to:
auto lhsInclusivenessAsUpper = lhs.IsUpper == lhs.IsInclusive;
auto rhsInclusivenessAsUpper = rhs.IsUpper == rhs.IsInclusive;Such warnings may be frequent in any project (YTSaururs has eight of them). It's up to the developers to decide what to do with them. Some may think that the version before shortening is clearer, others say the shortened code is better.
That's mostly about personal preferences. However, the PVS-Studio analyzer can detect such code, which is certainly handy. If you don't need such warnings in your code, you can always disable them.
Fragments N6, N7
void Reconfigure(TDistributedThrottlerConfigPtr config) override
{
MemberClient_->Reconfigure(config->MemberClient);
DiscoveryClient_->Reconfigure(config->DiscoveryClient);
auto oldConfig = Config_.Acquire();
if (oldConfig->LimitUpdatePeriod != config->LimitUpdatePeriod)
{
UpdateLimitsExecutor_->SetPeriod(config->LimitUpdatePeriod); // <=
}
if (oldConfig->LeaderUpdatePeriod != config->LeaderUpdatePeriod)
{
UpdateLeaderExecutor_->SetPeriod(config->LimitUpdatePeriod); // <=
}
....
Config_.Store(std::move(config));
}The analyzer warning: V778 Two similar code fragments were found. Perhaps, this is a typo and 'LeaderUpdatePeriod' variable should be used instead of 'LimitUpdatePeriod'. distributed_throttler.cpp:942, distributed_throttler.cpp:939
An error is haunting the code — the error of copy-paste.
In this case, it is not clear whether the LimitUpdatePeriod field is used correctly in the second case or whether the LeaderUpdatePeriod field should be used there. Who knows... :-/
Errors like this are common in any project, here is just another example:
static void xmlParseCommentComplex(xmlParserCtxtPtr ctxt, xmlChar *buf,
size_t len, size_t size)
{
....
q = CUR_CHAR(ql);
....
if (!IS_CHAR(q))
{
xmlFatalErrMsgInt(ctxt, XML_ERR_INVALID_CHAR,
"xmlParseComment: invalid xmlChar value %d\n",
q);
....
}
....
r = CUR_CHAR(rl);
....
if (!IS_CHAR(r))
{
xmlFatalErrMsgInt(ctxt, XML_ERR_INVALID_CHAR,
"xmlParseComment: invalid xmlChar value %d\n",
q); // <=
xmlFree (buf);
return;
}
....
}The analyzer warning: V778 Two similar code fragments were found. Perhaps, this is a typo and 'r' variable should be used instead of 'q'. parser.c:4771:1
Most likely, the q variable is written in the body of the if(!IS_CHAR( r )) condition by mistake, while the r variable should be there instead. Perhaps, but not for sure. We can endlessly discuss this "copy-paste problem" among developers, but the reality is that everyone, in any industry, with every tool, can make mistakes due to fatigue or distractions. Here, for example, are a hammer and a finger coming face-to-face. You're hammering a nail, and then bang! Your cursing instantly broadens the vocabulary of everyone around you. You are upset, hurt and ashamed. Hahaha, classic.
Be careful and get more rest!
Fragments N8, N9
....
std::vector<std::unique_ptr<THorizontalBlockWriter>> BlockWriters_;
....
class TPartitionMultiChunkWriter
: public TSchemalessMultiChunkWriterBase
{
....
TPartitionMultiChunkWriter(....) : ....
{
....
for (int partitionIndex = 0;
partitionIndex < partitionCount;
++partitionIndex)
{
BlockWriters_.emplace_back(
new THorizontalBlockWriter(Schema_, BlockReserveSize_)
);
....
}
....
}
}The analyzer warning: V1023 A pointer without owner is added to the 'BlockWriters_' container by the 'emplace_back' method. A memory leak will occur in case of an exception. schemaless_chunk_writer.cpp:1374
Here is a curious case of a potential memory leak invisible to the eye. Let's see why it may happen:
You can fix it in two ways.
Way N1. Since C++14, if only public constructors of an object are called, you can use the std::make_unique function:
BlockWriters_.emplace_back(
std::make_unique<THorizontalBlockWriter>(Schema_, BlockReserveSize_)
);Way N2. If the std::make_unique function is not available (due to the standard version or access to private constructors), you should wrap the raw pointer into a smart pointer before passing it to the vector:
std::unique_ptr<THorizontalBlockWriter> ptr {
new THorizontalBlockWriter(Schema_, BlockReserveSize_)
};
BlockWriters_.emplace_back(std::move(ptr));I also recommend calling push_back instead of emplace_back. Since a smart pointer is now being passed to the vector, there will be a call to the move constructor on the object inside the vector anyway. In the case of emplace_back, the compiler will do extra work to instantiate the variadic function template. With push_back, the compiler will do less work.
Here is another example of the code with a similar problem.
TBatch* GetBatch()
{
if (IsNewBatchNeeded())
{
Batches_.emplace_back(new TBatch()); // <=
}
return Batches_.back().get();
}The analyzer warning: V1023 A pointer without owner is added to the 'Batches_' container by the 'emplace_back' method. A memory leak will occur in case of an exception. tablet_request_batcher.cpp:244:1
Fragment N10, N11
std::vector<TSubquery> BuildThreadSubqueries(....)
{
....
std::vector<TSubquery> subqueries;
....
for (auto& stripe : subquery.StripeList->Stripes)
{
size_t operandIndex = stripe->GetInputStreamIndex();
YT_VERIFY(operandIndex >= 0);
....
}
....
}The analyzer warning: V547 Expression 'operandIndex >= 0' is always true. Unsigned type value is always >= 0. subquery.cpp:1061
Before we continue, I suggest looking at the YT_VERIFY macro:
#define YT_VERIFY(expr) \
do { \
if (Y_UNLIKELY(!(expr))) { \
YT_ASSERT_TRAP("YT_VERIFY", #expr); \
} \
} while (false)I also add the YT_ASSERT_TRAP macro for clarification:
#define YT_ASSERT_TRAP(trapType, expr) \
::NYT::NDetail::AssertTrapImpl(TStringBuf(trapType), TStringBuf(expr),
__SOURCE_FILE_IMPL__.As<TStringBuf>(),
__LINE__, TStringBuf(__FUNCTION__)); \
Y_UNREACHABLE()The YT_VERIFY macro is kind of assert on steroids, which does not go away even in release builds.
The analyzer reports that the first check — YT_VERIFY(operandIndex >= 0) — is always true. As you can see from the line above, the operandIndex variable is of size_t type, i.e., unsigned. And as we know, unsigned types do not contain a negative range of values. I can assume that at some point the GetInputStreamIndex function returned a value of a signed type, and such a check took place.
And here is another similar example where an unsigned long variable is checked for null:
TChunkIndexBlock BuildChunkIndexBlock( .... )
{
....
for(int sectorIndex = 0;
sectorIndex < bestFormatDetail->GetSectorCount();
++sectorIndex)
{
....
auto paddingSize = THashTableChunkIndexFormatDetail::SectorSize –
(buffer - sectorStart) - sizeof(TChecksum);
YT_VERIFY(paddingSize >= 0);
WriteZeroes(buffer, paddingSize);
....
}
....
}The analyzer warning: V547 Expression 'paddingSize >= 0' is always true. Unsigned type value is always >= 0. chunk_index_builder.cpp:278:1
Let's take a closer look at the paddingSize declaration:
auto paddingSize = THashTableChunkIndexFormatDetail::SectorSize –
(buffer - sectorStart) - sizeof(TChecksum);The developers may have intended that signed numbers would take precedence, and as a consequence, paddingSize would be of a signed type. But the sizeof(TChecksum) expression has the size_t type. Note this line.
Fragment N12
size_t TParallelFileReader::DoReadWithCallback(void* ptr,
size_t size,
DoReadCallback&& callback)
{
....
std::optional<TBlob> curBlob;
while (curBlob = ReadNextBatch())
{
....
}
if (....)
{
return curIdx;
}
else
{
size_t prevIdx = curIdx - curBlob->Size(); // <=
Y_VERIFY(!BatchTail_);
Y_VERIFY(curBlob.has_value()); // <=
BatchTail_ = curBlob->SubBlob(size - prevIdx, curBlob->Size());
return size;
}
}The analyzer warning: V1007 The value from the potentially uninitialized optional 'curBlob' is used. Probably it is a mistake. parallel_file_reader.cpp:241
To understand this code fragment better, we expand the Y_VERIFY macro:
#define Y_VERIFY(expr, ...) \
do{ \
if(Y_UNLIKELY(!(expr)))
{ \
::NPrivate::Panic(__SOURCE_FILE_IMPL__, __LINE__, __FUNCTION__,
#expr, " " __VA_ARGS__); \
} \
} while (false)Let's get into it. The code has the curBlob variable of optional type. Next, the ReadNextBatch function, which initializes curBlob, is called in a loop. The loop exits in two cases:
Suppose the loop exits for the second reason, and the control flow enters the else branch. Then, when the initializer of the prevIdx variable is evaluated, the null object is accessed, which leads to undefined behavior.
Look at the code two lines below. It seems that the developer anticipated that this might happen, so they added this check. But it was too late, the horse had already bolted.
I would suggest fixing the code as follows, by moving the check above the dereference:
Y_VERIFY(!BatchTail_);
Y_VERIFY(curBlob.has_value());
size_t prevIdx = curIdx - curBlob->Size();Fragment N13
template <bool IsSigned>
class TInteger128 { .... };
using ui128 = TInteger128<false>;
using i128 = TInteger128<true>;
// specialize std templates
namespace std
{
....
constexpr bool signbit(const ui128 arg) noexcept
{
Y_UNUSED(arg);
return false;
}
constexpr bool signbit(const i128 arg) noexcept
{
return GetHigh(arg) & 0x8000000000000000;
}
constexpr ui128 abs(const ui128 arg) noexcept
{
return arg;
}
constexpr i128 abs(const i128 arg) noexcept
{
return signbit(arg) ? (-arg) : arg;
}
}The analyzer warning: V1061 Extending the 'std' namespace may result in undefined behavior. int128.h:378:1
We love C++ and its standards! Standards help us understand how a particular tool is supposed to work. The C++ Standards Committee has established the rules for extending the std namespace. These rules state that the namespace cannot be extended by declaring functions, variables, or classes, as this can lead to undefined behavior.
The above example shows that the developer wanted to add overloads of standard functions for a 128-bit integer (TInteger128). However, this extending leads to undefined behavior. The correct place to add such functions is in the same namespace where the class is declared. Then at each place where a standard function is needed, its unqualified name should be used:
template <typename T>
void foo(T &arg)
{
using std::abs;
arg = abs(arg);
....
}Wrong std namespace extensions are so frequent that we have a ranking, and std::swap and std::hash are at the top of it. Speaking of them, actually.
Fragment N14.
namespace std
{
/////////////////////////////////////////////////////////////////////
template <class T, size_t N>
void swap(NYT::TCompactVector<T, N>& lhs,
NYT::TCompactVector<T, N>& rhs)
{
lhs.swap(rhs);
}
/////////////////////////////////////////////////////////////////////
template <class T, size_t N>
struct hash<NYT::TCompactVector<T, N>>
{
size_t operator()(const NYT::TCompactVector<T, N>& container) const
{
size_t result = 0;
for (const auto& element : container)
{
NYT::HashCombine(result, element);
}
return result;
}
};
/////////////////////////////////////////////////////////////////////
}The analyzer warning: V1061 Extending the 'std' namespace may result in undefined behavior. compact_vector-inl.h:999:1
Speaking of the custom std::swap function template, the C++ standard forbids adding new templates to the std namespace. This function should be moved to the NYT namespace
There is nothing wrong with the std::hash class template specialization in this case. But it can be taken outside the std namespace. At least, to avoid enticing other developers to continue the tradition of extending std:
template <class T, size_t N>
struct std::hash<NYT::TCompactVector<T, N>> { .... };Fragment N15
IUnversionedRowBatchPtr
THorizontalSchemalessRangeChunkReader::Read(
const TRowBatchReadOptions& options)
{
TCurrentTraceContextGuard traceGuard(TraceContext_);
auto readGuard = AcquireReadGuard(); // <=
....
}The analyzer warning: V1002 The 'TTimerGuard' class, containing pointers, constructor and destructor, is copied by the automatically generated copy constructor. schemaless_chunk_reader.cpp:731
In this case, the analyzer has detected the TTimerGuard, class template that may work incorrectly. This class template functions as an RAII wrapper over the timers. Let's look at its implementation:
template <class TTimer>
class TTimerGuard
{
public:
explicit TTimerGuard(TTimer* timer);
~TTimerGuard();
private:
TTimer* const Timer_;
};template <class TTimer>
TTimerGuard<TTimer>::TTimerGuard(TTimer* timer)
: Timer_(timer)
{
Timer_->Start();
}
template <class TTimer>
TTimerGuard<TTimer>::~TTimerGuard()
{
Timer_->Stop();
}As you can see, the constructor implements capturing the timer and starting it, while the destructor is responsible for stopping it. However, this class template will also have an implicit copy constructor/ operator generated. This may cause the object to be copied. And then it will cause the stop function to be called multiple times on the same timer. This won't necessarily cause an error, but the design of such a class template shouldn't give that any chance.
If this class template should have semantics similar to std::lock_guard, it will be enough to make it uncopiable:
template <class TTimer>
class TTimerGuard
{
public:
explicit TTimerGuard(TTimer* timer);
TTimerGuard(const TTimerGuard &) = delete;
TTimerGuard& operator=(const TTimerGuard &) = delete;
~TTimerGuard();
private:
TTimer* const Timer_;
};If objects are required to be movable (e.g., like std::unique_lock), the implementation needs to be changed a bit:
template <class TTimer>
class TTimerGuard
{
public:
TTimerGuard() = default;
explicit TTimerGuard(TTimer* timer) noexcept(noexcept(timer->Start()));
TTimerGuard(TTimerGuard &&other) noexcept;
TTimerGuard& operator=(TTimerGuard &&other) noexcept;
~TTimerGuard();
private:
TTimer* Timer_ = {};
};
template <typename TTimer>
TTimerGuard<TTimer>::TTimerGuard(TTimer *timer)
noexcept(noexcept(timer->Start()))
: Timer_ { timer }
{
if (Timer_)
Timer_->Start();
}
template <typename TTimer>
TTimerGuard<TTimer>::TTimerGuard(TTimerGuard &&other) noexcept
: Timer_ { std::exchange(other.Timer_, {}) }
{
}
template <typename TTimer>
TTimerGuard<TTimer>&
TTimerGuard<TTimer>::operator=(TTimerGuard &&other) noexcept
{
Timer_ = std::exchange(other.Timer_, {});
return *this;
}
template <typename TTimer>
TTimerGuard<TTimer>::~ TTimerGuard()
{
if (Timer_)
Timer_->Stop();
}Fragment N16
TDivisors GetDivisors(const TSchemaColumns& columns,
int keyIndex,
TConstExpressionPtr expr)
{
....
if (binaryOp->Opcode == EBinaryOp::Divide)
{
....
}
else if (binaryOp->Opcode == EBinaryOp::RightShift)
{
TUnversionedValue value = literal->Value;
value.Data.Uint64 = 1 << value.Data.Uint64; // <=
value.Id = 0;
return TDivisors{value};
}
....
}The analyzer warning: V629 Consider inspecting the '1 << value.Data.Uint64' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. range_inferrer.cpp:378
Such code is considered potentially dangerous. The literal 1 of the int type is shifted to the left by value.Data.Unit64. As a result, the value of such an expression may not be defined. If value.Data.Uint64 is within the range of [32 ... 63], a sign overflow occurs. The bitwise shift operator will not change the type of the resulting expression, which will still be of the int type. Even though the right operand is already of the right type. The proof.
The analyzer indicates such a range because the resulting expression is then being cast to a larger type. Fixing the problem is quite easy, you just need to cast the 1 literal to the correct type:
value.Data.Uint64 = (uint64_t) 1 << value.Data.Uint64;You may ask, "What's so dangerous about it? Will it explode or won't start?"
The answer is, "It will start..."
Most likely, everything is working fine now and will continue to do so if overflow is avoided. But it can "explode", actually. There's no point in guessing how an error will manifest itself when you enter the territory of undefined behavior. And if you really want to, I suggest you read a great article on this topic: "Falsehoods programmers believe about undefined behavior".
The developers of the project took safety seriously. They implemented many additional checks and were careful enough to provide comments that clarify the logic of the code. The code is all clear and beautiful. This truly deserves our respect.
It's okay to make mistakes. A good programmer should know how to write good code and how to avoid writing bad one. I hope you learned something new or even helpful from this code fragment examination, analyzer warnings, and code faults.
As usual, I suggest trying the PVS-Studio analyzer. We have a free license for open-source projects.
Take care, and have a good day!
0
0
0
55
0
0